Fix GetProcessBaseAddress to Remove File Comparison |
||
Issue descriptionGetProcessBaseAddress is just a pain. The previous implementation using undocumented register allocation in the initial thread context was fragile and subject to failure with third party software. The new implementation I wrote which was supposed to only use documented mechanisms has proven to be unreliable as well due to the mismatch between the image paths and the process path (due to things like 8.3 file names and the image path becoming *sticky* after renames). So I propose to replace it will the undocumented, but reasonably unlikely to change, reading of the ImageBaseAddress field from the process PEB. This is situated at address 8 in 32bit and 16 in 64 bit and stores a pointer value which seems to correspond with the image load address (and the name sort of gives it away). Hopefully this might finally get rid of the problem until we can come up with a better solution which doesn't rely on any memory state of the new process at all. If there's any concerns with doing that let me know.
,
Aug 7 2017
If it works it would completely remove the requirement to match paths and so should fix 744638 as well (I alluded to the issue with 8.3 file names above). pennymac@'s changes to handle device paths with short paths is still a useful change to make though and IMO should still be completed. In an ideal world we wouldn't be passing variables in exported values between the parent and child but instead passing them through some more structured mechanism but that's a task for another day.
,
Aug 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f398005bc4ca0cc2dab2198faa99d4ee8f4da60d commit f398005bc4ca0cc2dab2198faa99d4ee8f4da60d Author: James Forshaw <forshaw@chromium.org> Date: Tue Aug 15 09:29:46 2017 Reimplemented GetProcessBaseAddress This patch changes the implementation of GetProcessBaseAddress to extract the image base from the PEB. This should be quicker and hopefully more reliable than the previous implementation using file listing. Bug: 753140 Cq-Include-Trybots: master.tryserver.chromium.win:win10_chromium_x64_rel_ng Change-Id: Ic4b7ae2af57ce980fb007cda4a3fbe63c4dfc5bf Reviewed-on: https://chromium-review.googlesource.com/608702 Reviewed-by: Penny MacNeil <pennymac@chromium.org> Reviewed-by: Will Harris <wfh@chromium.org> Commit-Queue: James Forshaw <forshaw@chromium.org> Cr-Commit-Position: refs/heads/master@{#494349} [modify] https://crrev.com/f398005bc4ca0cc2dab2198faa99d4ee8f4da60d/sandbox/win/src/nt_internals.h [modify] https://crrev.com/f398005bc4ca0cc2dab2198faa99d4ee8f4da60d/sandbox/win/src/win_utils.cc [modify] https://crrev.com/f398005bc4ca0cc2dab2198faa99d4ee8f4da60d/sandbox/win/src/win_utils.h [modify] https://crrev.com/f398005bc4ca0cc2dab2198faa99d4ee8f4da60d/sandbox/win/src/win_utils_unittest.cc
,
Aug 15 2017
FYI: first hit trunk in M62, and shipped to canary starting in 62.0.3187.X (Wed, 16 Aug).
,
Aug 16 2017
|
||
►
Sign in to add a comment |
||
Comment 1 by wfh@chromium.org
, Aug 7 2017