src/rufus: Prevent assertion failure if START is clicked while image_path is NULL

[sehawq]

This commit adds a defensive check to ensure that clicking the START button when no image has been selected (e.g. if the UI accidentally enables it or is in an inconsistent state) does not trigger an assertion failure. Closes #2923.

[pbatard]

Do you have replicable steps to enable this situation.

I'd much rather fix the root cause (that leaves the UI in such a state) than take defensive measures.

In short I am asking what prompted you to submit this patch all of a sudden, because this must stem from something you encountered. If so, what was it and how exactly did it come about?

[pbatard]

Also note that the assert statements I leave in Rufus are precisely so that people will report on situations that I very much try to design the code not to happen, and want to make sure that, if they do happen, I hear about it so that I can fix it at the source. Else, I would add the kind of silent failure you suggest. But the problem is that they give me exactly zero insight on potential issues that should be addressed.

[sehawq]

Hi Pete,

I've been trying to consistently reproduce the issue on my end. While I haven't been able to trigger the exact "active START button with no image selected" state every single time, it's clear from the reports that a race condition or a logic flaw exists in the UI refresh sequence—especially after interacting with the Download dialog or switching between boot selection modes.

Since you've confirmed this state should be impossible by design, I’d like to move forward by examining the UI update logic in the source code. I want to ensure that whenever the Boot Selection is set to "Disk or ISO", the START button is strictly tied to a valid image_path check, regardless of previous states like "Non bootable".

Could you point me to the specific function where Rufus handles the global UI state refresh (e.g., where EnableWindow is called for the START button)? I’d rather submit a fix that reinforces this logic than my previous defensive check.

[sehawq]

Hi Pete,

That’s a fair point — I understand why you keep asserts in place, and I agree a “silent guard” isn’t a proper fix if this state should be impossible.

What prompted the PR was user reports of hitting the “START clicked while image_path is NULL” assert, i.e. START being enabled in “Disk or ISO” without a selected image. On my side I haven’t managed to get a 100% deterministic repro yet, but the state seems most likely to appear after UI state transitions, especially:

• toggling Boot selection between “Non bootable” and “Disk or ISO”
• opening the Download dialog and then cancelling/closing it
• switching image/boot selection modes quickly and returning to the main dialog

I’ll rework this PR to focus on the root cause by enforcing the invariant in the central UI refresh path (where START is enabled/disabled): when Boot selection is “Disk or ISO” (BT_IMAGE), START must only be enabled iff image_path is valid, regardless of previous states. If needed I can also add temporary debug trace (only in debug builds) to capture the sequence of state changes leading to START being enabled unexpectedly.

If you can point me to the function(s) responsible for the main UI refresh / EnableWindow logic for START, I’ll update the patch accordingly.

[pbatard]

Thanks, I do appreciate the help on this, and I definitely would like what's clearly a bug in our UI logic fixed.

There are quite a few functions responsible for the UI refresh, mostly located in rufus.c, like SetAllowedFileSystems(), SetBootOptions(), SetPartitionSchemeAndTargetSystem(), SetFileSystemAndClusterSize() SetProposedLabel(), ToggleImageOptions() as well EnableControls() that toggle which UI element should be enabled or not. As you will see this latter function is called all over the place. Then there's case IDC_BOOT_SELECTION: in the message loop that handles the Boot Selection control and calls on the above to decide whether the START and other related controls should be enabled or not.

I'll be the first to admit that the logic on these calls is a mess to try to follow, because there are so many elements at play on whether some elements should be enabled/disabled and when that they were mostly added from trial and error. I sure wish AI was smart enough that you could feed it code like Rufus' and it would understand when each UI element needs to be enabled/disabled and create a nice state machine that handles all the conditions for you, because maintaining these conditions manually is a major PITA and I added asserts precisely because I was pretty sure I would screw it up... 😉

I think, since we know that the assert is about image_path having gotten NULL whereas it was probably valid before (since, if the user didn't change the Boot Selection option, START should only have gotten enabled if there was a valid image), we probably want to concentrate on following the parts of the code that NULLed image_path to determine the sequence of user interaction that triggers the assert. My guess is that we're probably missing an EnableControls() call somewhere, after a path of the code that may return a NULL image_path, so we need to look at the actual image_path assignations (as well as the safe_free(image_path) calls that also NULL the variable).

My current guess, since I'm seeing mentions of this being triggered after trying to download an ISO, is that the culprit might be the image_path = safe_strdup(img_save.ImagePath); in net.c's DownloadISOThread(), though that would then mean that it's the _strdup() call itself that returned NULL as we do check for img_save.ImagePath == NULL before, and return an error if that is the case.

I guess one good test, if you have VS2022 installed, would be to alter the code to make each of these separate assignation to set image_path to NULL, and see if you can trigger the assert.

[sehawq]

Thanks for the detailed guidance! I really appreciate you taking the time to explain the root cause perspective.

I followed your advice and analyzed src/net.c (specifically DownloadISOThread), but it seems to handle the cleanup correctly by invoking SendMessage(hMainDialog, UM_ENABLE_CONTROLS, ...) at the end via the out label, which ensures EnableControls is called even if safe_strdup fails.

However, while tracing other image_path assignments in src/rufus.c, I found a suspicious path in the WM_DROPFILES handler (around line 2961) that matches your theory perfectly:

// In rufus.c, inside WM_DROPFILES handler:

safe_free(image_path);                 // 1. image_path becomes NULL (freed)
image_path = wchar_to_utf8(wbuffer);   // 2. Try to allocate new path
safe_free(wbuffer);

if (image_path != NULL) {
    img_provided = TRUE;
    // Simulate image selection click -> This eventually calls EnableControls()
    SendMessage(hDlg, WM_COMMAND, IDC_SELECT, 0);
}
// 3. BUG: If wchar_to_utf8 returns NULL (allocation failure), we skip the if block.
// EnableControls() is NEVER called here.
// Result: image_path is NULL, but the UI (START button) remains ENABLED from the previous valid state.

I believe this is a strong candidate for where the invariant is broken. If wchar_to_utf8 fails, we end up with a NULL image_path but an enabled START button.

I propose adding an else block to handle this case and enforce the UI update:

if (image_path != NULL) {
    img_provided = TRUE;
    SendMessage(hDlg, WM_COMMAND, IDC_SELECT, 0);
} else {
    // Allocation failed, ensure UI reflects the empty image_path
    EnableControls(TRUE, FALSE);
}

Shall I proceed with updating the PR to include this fix instead of the defensive check?

[pbatard]

That's definitely something that should be fixed indeed. Nice find!

Feel free to update the PR to fix this.

[sehawq]

Updated.

[sehawq]

I've updated the PR to focus solely on the root cause fix as suggested.

• Removed the defensive check (if ((boot_type == BT_IMAGE) && (image_path == NULL))) to avoid masking the underlying issue.
• Kept the fix in WM_DROPFILES, which ensures EnableControls(TRUE, FALSE) is called when image_path becomes NULL (e.g., invalid drop or conversion failure), properly resetting the UI state.

This should address the UI logic inconsistency directly without needing extra safeguards.

[sehawq]

Is there any progress? @pbatard

[pbatard]

I'm working on something, and I want to complete that before I apply your request. Please be patient, okay.