GetMappedFileNameW returns path with 8.3 segments, sandbox fails to start |
||||||||||||
Issue descriptionsandbox::GetProcessBaseAddress inderectly calls ::QueryFullProcessImageNameW and ::GetMappedFileNameW functions in order to locate the address of the main exe module in memory of a process. But they can return inconsistent filepaths. On several win10 buildbots we observe GetMappedFileNameW returning filepath with a 8.3 path segment: \Device\HarddiskVolume2\Users\CHROME~2\AppData\Local\Temp\scoped_dir4840_27583\some_tool_name.exe QueryFullProcessImageNameW returns normal path: \Device\HarddiskVolume2\Users\chrome-bot\AppData\Local\Temp\scoped_dir4840_27583\some_tool_name.exe Due to this inconsistency GetProcessBaseAddress "fails" to locate the exe module, causing sandbox::TargetProcess::Create to fail to spawn target process. I observe this problem on M59, but it looks like the bug is still there in M61 as well.
,
Jul 18 2017
Issue 743038 has been merged into this issue.
,
Jul 18 2017
,
Jul 18 2017
,
Jul 18 2017
,
Jul 20 2017
,
Jul 27 2017
FYI Veranika: not such a simple fix, but a patch is getting close. https://chromium-review.googlesource.com/c/580629
,
Aug 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0a29c3285746c1a80ca039e7887180fc3f3e159c commit 0a29c3285746c1a80ca039e7887180fc3f3e159c Author: Penny MacNeil <pennymac@chromium.org> Date: Wed Aug 09 17:16:41 2017 [Windows Sandbox] Possible short/8.3 form native file paths prevent startup. win_utils.cc, GetProcessBaseAddress() ends up comparing file paths from two system APIs: ::QueryFullProcessImageName() and ::GetMappedFileName(). Both APIs return native file paths. However, depending on filesystem, these paths can be in short/8.3 form. Path comparison in GetProcessBaseAddress can fail, preventing process startup. This CL does the following: - GetProcessBaseAddress() now uses sandbox::ConvertToLongPath(). - sandbox::ConvertToLongPath() now handles extending native device paths. TESTS=sbox_unittests.exe BUG= 744638 Cq-Include-Trybots: master.tryserver.chromium.win:win10_chromium_x64_rel_ng Change-Id: I86cb8b4c8da13f4405aba16e127c512a1eec539d Reviewed-on: https://chromium-review.googlesource.com/580629 Reviewed-by: James Forshaw <forshaw@chromium.org> Commit-Queue: Penny MacNeil <pennymac@chromium.org> Cr-Commit-Position: refs/heads/master@{#493034} [modify] https://crrev.com/0a29c3285746c1a80ca039e7887180fc3f3e159c/sandbox/win/src/win_utils.cc [modify] https://crrev.com/0a29c3285746c1a80ca039e7887180fc3f3e159c/sandbox/win/src/win_utils.h [modify] https://crrev.com/0a29c3285746c1a80ca039e7887180fc3f3e159c/sandbox/win/src/win_utils_unittest.cc
,
Aug 9 2017
Veranika, this has landed, and will start shipping in tomorrow's canary. I'm setting to fixed, but a verification from you would be appreciated. M62, branch 3181.
,
Aug 9 2017
Thanks a lot! I'll try to test it today.
,
Aug 11 2017
I've tried it, and it worked. Thanks!
,
Aug 11 2017
Ok, it worked on affected bots, but failed on previously not affected bots. On them, |process_path| contained short segment. Luckily, a one-line fix is required: a call to |ConvertToLongPath| after |GetProcessPath| in line 583: https://cs.chromium.org/chromium/src/sandbox/win/src/win_utils.cc?rcl=a57a8b2195e2853a143999a0004278d1e4bd3a32&l=583 I'll try to submit a CL to fix that.
,
Aug 14 2017
<sigh> So my comment about this was prescient: https://cs.chromium.org/chromium/src/sandbox/win/src/win_utils.cc?rcl=a57a8b2195e2853a143999a0004278d1e4bd3a32&l=580 Veranika, if you haven't already put time into this, I'll just get forshaw@ to do this tiny adjustment in an active cl: https://chromium-review.googlesource.com/c/608702
,
Aug 14 2017
,
Aug 14 2017
Looks like I'll remove the old implementation completely so this patch won't be needed. I'm confident this version should fill all varanika@'s issues as it doesn't rely on any file path comparisons.
,
Aug 15 2017
veranika@ I've committed the replacement for the GetProcessBaseAddress function which doesn't do any file path comparisons. If you find the time could you possibly test it (when it becomes available) to make sure it fixes all the cases you've found?
,
Aug 15 2017
Fixed (again) in https://bugs.chromium.org/p/chromium/issues/detail?id=753140. Please verify at your leisure Veranika.
,
Aug 16 2017
I ran this patch on bots both affected and unaffected by the bug before, and it worked for me. |
||||||||||||
►
Sign in to add a comment |
||||||||||||
Comment 1 by veranika@chromium.org
, Jul 17 2017