Repository navigation
[NETSH] Compare the stack entry against the tail, not the head twice - #9473
Conversation
github-project-automation
Bot
moved this from New PRs
to Approved by reviewers
in ReactOS PRs
binarymaster
left a comment
binarymaster
left a comment
There was a problem hiding this comment.
Hello,
Thanks for your interest in ReactOS!
We ask our contributors to use their full name when committing.
Your commits are authored correctly, however your GitHub account needs to be configured accordingly, so maintainers can use Squash & Merge button without getting the author field modified back to nickname by GitHub, it's done in two steps:
- Set your full name in GitHub profile settings (it should match your
user.namesetting) - Uncheck
Keep my email addresses privatein GitHub email settings
Regards,
github-project-automation
Bot
moved this from Approved by reviewers
to WIP / Waiting on contributor
in ReactOS PRs
|
Understood, the profile Name field says just "Dmitry" while the commits say "Dmitry Rantovov". I will set it to the full name so Squash & Merge keeps the author, and I will ping here once it is done rather than force pushing anything. |
|
Profile name is now the full "Dmitry Rantovov", so a squash merge will keep the author the commit already has. The email on the commit is linked to the account too, that was missing before and it is why the commit showed as unattributed. |
|
@darkdi can confirm the name is set correctly now. However email step is still needed: 
|
|
Done, the setting is off now. The commit here is authored as Dmitry Rantovov [email protected] and that address is verified on the account, so a squash merge should keep it as is. |
github-project-automation
Bot
moved this from WIP / Waiting on contributor
to Approved by reviewers
in ReactOS PRs
github-project-automation
Bot
moved this from Approved by reviewers
to Done
in ReactOS PRs
|
Thanks! |
…eactos#9473) RemoveContextFromStack tests pStackEntry == pContextStackHead twice in the same condition, so the first branch takes every entry that happens to be the head, not only the one that is head and tail at once. The branch below it, the one that relinks pStackEntry->pNext->pPrev and moves the head forward, is then unreachable. Removing the head of a stack that has more than one entry sets both head and tail to NULL and frees only that entry, so the remaining entries are lost and leaked. The body of the first branch is what says which test was meant: it clears both ends, and that is only correct for the single element case. Compare the second test against pContextStackTail. I found this by reading, not by hitting it in a running netsh session, so I have not confirmed the leak on a live context stack. Reviewed-by: Whindmar Saksit <[email protected]> Reviewed-by: Hermès Bélusca-Maïto <[email protected]> Reviewed-by: Eric Kohl <[email protected]> Signed-off-by: Dmitry Rantovov <[email protected]> (cherry picked from commit b91b138)
Purpose
RemoveContextFromStacktestspStackEntry == pContextStackHeadtwice in the same condition, so the first branch takes every entry that happens to be the head, not only the one that is head and tail at once. The branch below it, the one that relinkspStackEntry->pNext->pPrevand moves the head forward, is then unreachable. Removing the head of a stack that has more than one entry sets both head and tail to NULL and frees only that entry, so the remaining entries are lost and leaked.The body of the first branch is what says which test was meant: it clears both ends, and that is only correct for the single element case.
Proposed changes
pContextStackTailI found this by reading, not by hitting it in a running netsh session, so I have not confirmed the leak on a live context stack.