fix: value-initialize NOTIFYICONDATA and clear hIcon after destroy - #103
Open
JulianPscheid wants to merge 1 commit into
Open
fix: value-initialize NOTIFYICONDATA and clear hIcon after destroy#103JulianPscheid wants to merge 1 commit into
JulianPscheid wants to merge 1 commit into
Conversation
nid and niif were declared without initializers. _ApplyIcon() backs up nid.szTip before zeroing the struct and sets NIF_TIP when the first byte is non-null, so on the first setIcon() call it reads uninitialized memory and can register it as the tooltip. An app that never calls setToolTip() then shows whatever happened to be in that buffer. Destroy() and the WM_DESTROY handler also left nid.hIcon dangling after DestroyIcon(), so the null check in SetIcon() could never fire and a destroy/re-init cycle destroyed the same handle twice.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two problems in the Windows plugin, both stemming from
nidnever being initialized.Uninitialized szTip becomes the tooltip
nidandniifare declared without initializers. On the firstsetIcon()call,_ApplyIcon()copiesnid.szTipinto a backup before it zeroes the struct, restores it afterwards, then setsNIF_TIPif the first byte is non-null:That backup reads uninitialized memory. When those bytes happen to be non-zero,
NIF_TIPgets set and Windows renders them as the tooltip. One of our users hovered the tray icon and saw a truncated copy of theirPATH. Apps that callsetToolTip()overwrite the garbage and never notice, but an app that never sets a tooltip will show whatever was in that memory.hIcon left dangling after DestroyIcon
Destroy()and theWM_DESTROYhandler both callDestroyIcon(nid.hIcon)without clearing the handle afterwards.SetIcon()checksnid.hIcon != nullptrbefore destroying the old icon, but since nothing ever sets it back to null, a destroy followed by re-initialization callsDestroyIcontwice on the same handle. If Windows recycled that handle value in between, the second call frees an unrelated icon.Value-initializing both members fixes the first problem and makes the existing null check in
SetIcon()meaningful. ClearinghIconat both destroy sites fixes the second.