Skip to content

Fix crash on startup when no configuration exists yet - #102

Open
stefan-siebert wants to merge 1 commit into
0xeb:mainfrom
stefan-siebert:fix/shell-overlay-diag-null-config-root
Open

Fix crash on startup when no configuration exists yet#102
stefan-siebert wants to merge 1 commit into
0xeb:mainfrom
stefan-siebert:fix/shell-overlay-diag-null-config-root

Conversation

@stefan-siebert

Copy link
Copy Markdown

Sally 1.0.24 (x64) crashes on startup on a machine that has no Salamander configuration in the registry yet. No window ever appears, the access violation hits about 140 ms in. Since the crash happens before any configuration is written, the key it would need never gets created and it repeats on every launch, so the build is effectively unusable on a clean machine.

Cause

InitShellIconOverlays() snapshots the overlay diagnostics header and copies the resolved registry root into it (src/shiconov.cpp):

lstrcpynA(ShellOverlayDiag.Header.ConfigRoot, SALAMANDER_ROOT_REG,
          (int)sizeof(ShellOverlayDiag.Header.ConfigRoot));

SALAMANDER_ROOT_REG is NULL at that point whenever no configuration was loaded. FindLatestConfiguration() starts with loadConfiguration = NULL and only assigns a root when a key with a Configuration subkey actually exists, so it stays NULL on a first run — and also when an older configuration is found but the user declines the import offer. The fallback SALAMANDER_ROOT_REG = SalamanderConfigurationRoots[0] in sally_entry_lifecycle.cpp runs later in the startup sequence than the InitShellIconOverlays() call.

src/common/lstrfix.h deliberately remaps lstrcpyn to _sal_lstrcpynA so that NULL faults instead of being swallowed the way the kernel32 version would, which is why this is a hard crash rather than an empty string.

Every other reader of SALAMANDER_ROOT_REG in the tree already guards for NULL (main_window_commands_help.cpp, main_window_config_persistence.cpp, plugins_fs_encapsulation.cpp, sally_strings_waitwindow.cpp). This call site is new in c580010 and doesn't.

From the bug report

Sally 1.0.24 (x64)

Information About Exception:
  Exception: access violation: read on 0x0000000000000000

Call Stacks:
  Thread with Exception
  WinMainBody(...)
  WinMainBody::FindLatestConfiguration
  WinMainBody::StartupDialog
  WinMainBody::inicialization
  InitShellIconOverlays()

Icon Overlays:
  Config root in use:
  ANSI code page: 1252   Registered: 0   Loaded: 0

Salamander Started Before: 0:00:00:00.141

The code at the faulting RIP is the copy loop of _sal_lstrcpynA:

41 0F B6 14 09    movzx edx, byte ptr [r9+rcx]   ; *lpString2  <- faults, src == NULL
84 D2 / 74 0A     test dl,dl / jz done
88 11             mov byte ptr [rcx], dl         ; *lpString1++ = *lpString2++
48 FF C1          inc rcx
49 3B C8 / 72 ED  cmp rcx,r8 / jb loop
C6 01 00          mov byte ptr [rcx], 0

RCX points at 256 zero bytes in .data, i.e. the freshly Reset() ShellOverlayDiag.Header.ConfigRoot, and the empty Config root in use: line in the report says the same thing.

Repro

  1. reg delete "HKCU\Software\Sally\1.0" /f (or just use a machine that never had Salamander or Sally on it)
  2. Start sally.exe

With the guard in place the same machine starts normally and the report shows an empty config root, which is the correct answer for "no configuration loaded".

InitShellIconOverlays() copies SALAMANDER_ROOT_REG into the overlay
diagnostics header, but that pointer is NULL whenever
FindLatestConfiguration() didn't load a configuration - a first run on a
clean machine, or an import offer the user declined. It only gets set to
SalamanderConfigurationRoots[0] later in the startup sequence, after this
call.

lstrcpyn is remapped to _sal_lstrcpynA in common/lstrfix.h so that NULL
faults instead of being swallowed, so this is an access violation reading
0x0 about 140 ms into startup. It happens before any configuration is
written, so it repeats on every launch: 1.0.24 can't start at all on a
machine that has no Salamander configuration yet.

Guard the copy the way every other reader of SALAMANDER_ROOT_REG already
does. An empty config root in the report is the right answer for "no
configuration loaded".
@0xeb

0xeb commented Sep 1, 2026

Copy link
Copy Markdown
Owner

Thank you, Stefan. I did fix that a while back and just noticed your PR. Version 1.0.25 should already fix that. If not, let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants