Fix more UB in windows/system refresh_processes_specifics #1014
Conversation
| @@ -239,7 +240,6 @@ impl SystemExt for System { | |||
| if ntstatus == STATUS_INFO_LENGTH_MISMATCH { | |||
| // GetNewBufferSize | |||
| if cb_needed == 0 { | |||
| process_information.reserve(buffer_size); | |||
There was a problem hiding this comment.
Putting the reserve call here failed to reserve if cb_needed was >0
There was a problem hiding this comment.
This also would do nothing because the Vec already had capacity for buffer_size additional elements, it hasn't been doubled yet
LunNova
force-pushed
the
lunnova/refresh-process-ub
branch
2 times, most recently
from
ce50358
to
9410c73
Compare
LunNova
force-pushed
the
lunnova/refresh-process-ub
branch
from
9410c73
to
95bd5c5
Compare
src/windows/system.rs
Outdated
| @@ -227,6 +228,9 @@ impl SystemExt for System { | |||
|
|
|||
| loop { | |||
| let mut cb_needed = 0; | |||
| // reserve(n) ensures the Vec has capacity for n additional elements | |||
| // so we should reserve buffer_size - len | |||
| process_information.reserve(buffer_size - process_information.len()); | |||
There was a problem hiding this comment.
Unless I missed something, process_information's length is always 0 until we use set_len below. So why subtracting the length?
There was a problem hiding this comment.
That is true, but it would be pretty easy for a future change to this function to allow reaching this point with a length set as it's pretty long and hard to follow.
I'd feel more comfortable removing the len part if this function broke out of the loop immediately before the set_len. Would you be okay with a refactor like that?
|
Thanks for working on this! It looks good to me. Do you plan to add more things or can I merge once CI pass? |
|
I've tested that it doesn't cause "misaligned pointer dereference: address must be a multiple of 0x8" under wine but haven't tested it on native windows yet, can you confirm that's still good before merging? |
| let mut process_information_offset = 0; | ||
| loop { | ||
| let p = unsafe { | ||
| process_information |
There was a problem hiding this comment.
We could assert! that process_information_offset isn't too large here.
There was a problem hiding this comment.
I'll make a followup PR later with some more hardening.
Gonna do that in a few then. |
|
Tested locally on Windows and worked as expected. 👍 |
LunNova
changed the title
|
Thanks for fixing this bug! |
|
Can we have a release with this? (and maybe the current version yanked?) |
|
I'm planning to. Just waiting to hear from @granitDev about their bug. If it's not fixed by this PR, I'll need to make another patch. |
For #1009