New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 744638 link

Starred by 3 users

Issue metadata

Status: Verified
Owner:
Last visit > 30 days ago
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug



Sign in to add a comment

GetMappedFileNameW returns path with 8.3 segments, sandbox fails to start

Project Member Reported by veranika@chromium.org, Jul 17 2017

Issue description

sandbox::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.
 
Cc: joenotcharles@chromium.org
Issue 743038 has been merged into this issue.
Cc: -joenotcharles@chromium.org penny...@chromium.org
Cc: joenotcharles@chromium.org
Owner: penny...@chromium.org
Status: Assigned (was: Untriaged)
Status: Started (was: Assigned)
FYI Veranika: not such a simple fix, but a patch is getting close.
https://chromium-review.googlesource.com/c/580629 
Project Member

Comment 8 by bugdroid1@chromium.org, 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

Cc: forshaw@chromium.org
Status: Fixed (was: Started)
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.
Thanks a lot! I'll try to test it today.
Status: Verified (was: Fixed)
I've tried it, and it worked. Thanks!
Status: Assigned (was: Verified)
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.
Blockedon: 608702
<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 


Blockedon: -608702
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.

Comment 16 Deleted

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?
Status: Fixed (was: Assigned)
Fixed (again) in https://bugs.chromium.org/p/chromium/issues/detail?id=753140.

Please verify at your leisure Veranika.
Status: Verified (was: Fixed)
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