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

Issue 666992 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Last visit > 30 days ago
Closed: Dec 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug



Sign in to add a comment

[Windows][Host] Cannot connect to Windows 10 host in curtain mode.

Project Member Reported by joedow@chromium.org, Nov 19 2016

Issue description

This is a recent regression in M56 and needs to be fixed.

I've seen two occurrences of this and was able to get a repro.  It looks like the remoting_desktop process is not launched correctly (it exists with error code 0xC0000142 (STATUS_DLL_INIT_FAILED)).

 

Comment 1 by joedow@chromium.org, Nov 22 2016

After some additional investigation, it looks like that error is transient and is occurring for the helper process used to launch remoting_desktop.

The remoting_desktop process is failing with 'STATUS_ELEVATION_REQUIRED'.  I think this is related to the change made a month ago to require administrator rights, but I haven't confirmed this and it should be something which was already set when we were using GYP builds.

Comment 2 by joedow@chromium.org, Nov 22 2016

This does look to be related to my previous change (https://codereview.chromium.org/2370293002/).

I added the uiAccess flag as the old GYP builds specified it and it was missing in GN.  We are using a manifest fragment instead of command line argument so perhaps something is different there.

I would like to revert the manifest change and then test whether it is actually required.

Comment 3 by joedow@chromium.org, Nov 29 2016

After a bit more investigation, this problem was actually introduced by this change:
Use ChannelMojo between the remoting daemon and desktop processes.
https://codereview.chromium.org/2446053002

If I sync to the CL just before it, the UiAccess enabled desktop binary works fine.  When I roll forward to include this change, I can no longer connect.

I am investigating now to see what the root cause is.

Comment 4 by joedow@chromium.org, Nov 29 2016

Status: Started (was: Assigned)

Comment 5 by joedow@chromium.org, Nov 30 2016

I am going to check in my change to unblock M56 (need to verify whether removing UiAccess causes any problems).

I will continue investigating the problems from the Mojo conversion to see if we can fix them in M56 or if they will need to resolved in M57.
Project Member

Comment 6 by bugdroid1@chromium.org, Nov 30 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fdd9968f262f1fba0d9b2f3e0261226732e2ee67

commit fdd9968f262f1fba0d9b2f3e0261226732e2ee67
Author: joedow <joedow@chromium.org>
Date: Wed Nov 30 15:59:24 2016

Fixing a problem where curtain mode connections were failing on Windows 8+

I tracked this down to a change I made where I added the uiAccess flag to
the remoting_desktop binary (https://codereview.chromium.org/2370293002/).
Adding this manifest flag causes CreateProcess to fail with
STATUS_ELEVATION_REQUIRED when run in curtain mode.  I suspect it has
something to do with the process being started in the logon session.

I am reverting this flag to unblock this scenario (M56 needs this fix) and
will verify whether the flag is actually needed (it was originally added for
parity with GYP builds but may not be necessary).

BUG= 666992 

Review-Url: https://codereview.chromium.org/2521453005
Cr-Commit-Position: refs/heads/master@{#435272}

[modify] https://crrev.com/fdd9968f262f1fba0d9b2f3e0261226732e2ee67/remoting/host/win/BUILD.gn

Comment 7 by joedow@chromium.org, Nov 30 2016

Labels: Merge-Request-56

Comment 8 by dimu@chromium.org, Dec 1 2016

Labels: -Merge-Request-56 Merge-Approved-56 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M56 (branch: 2924)
Project Member

Comment 9 by bugdroid1@chromium.org, Dec 2 2016

Labels: -merge-approved-56 merge-merged-2924
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ffa8fec145e25ac6b34ee56c65d6b11eb9369c76

commit ffa8fec145e25ac6b34ee56c65d6b11eb9369c76
Author: Joe Downing <joedow@google.com>
Date: Fri Dec 02 02:47:40 2016

Fixing a problem where curtain mode connections were failing on Windows 8+

I tracked this down to a change I made where I added the uiAccess flag to
the remoting_desktop binary (https://codereview.chromium.org/2370293002/).
Adding this manifest flag causes CreateProcess to fail with
STATUS_ELEVATION_REQUIRED when run in curtain mode.  I suspect it has
something to do with the process being started in the logon session.

I am reverting this flag to unblock this scenario (M56 needs this fix) and
will verify whether the flag is actually needed (it was originally added for
parity with GYP builds but may not be necessary).

BUG= 666992 

Review-Url: https://codereview.chromium.org/2521453005
Cr-Commit-Position: refs/heads/master@{#435272}
(cherry picked from commit fdd9968f262f1fba0d9b2f3e0261226732e2ee67)

Review URL: https://codereview.chromium.org/2543133002 .

Cr-Commit-Position: refs/branch-heads/2924@{#281}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}

[modify] https://crrev.com/ffa8fec145e25ac6b34ee56c65d6b11eb9369c76/remoting/host/win/BUILD.gn

Here is the CL which originally added the UiAccess flag:
https://chromiumcodereview.appspot.com/10836224

The intention was to enable usage of SendSAS().  I will need to do some testing to see if this is still necessary as based on the MSDN docs, the manifest flag is not needed if the process is running as LocalSystem.
Project Member

Comment 11 by bugdroid1@chromium.org, Dec 13 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1cdb04810de9689ba0ea5ab7e70bd35ce5477063

commit 1cdb04810de9689ba0ea5ab7e70bd35ce5477063
Author: joedow <joedow@chromium.org>
Date: Tue Dec 13 06:11:15 2016

Fixing CRD curtain mode connection failures on Win8+ official builds

I've root caused this and the problem was introduced in this CL:
https://codereview.chromium.org/2446053002

That CL moved the logic which reported the worker process launch from the
IPC Channel Connected method to an IO completion port listener.  This works
fine on un-official builds, however it breaks on official builds.  The
reason for the failure is that the remoting_desktop process on official
builds are signed and include 'UiAccess' in their manifest.  When the
launcher process uses ShellExecuteEx to launch the worker process in this
scenario, we actually see two processes get launched sequentially.  The
first starts and exists with error code 'STATUS_ELEVATION_REQUIRED' and
the second launches with the correct permissions.  This behavior worked
fine before as we listened for the connection to the IPC channel which
was done by the second, successful process launch.  With the new code,
we observed the first process launch, set up the Mojo channel for it, and
tried waiting on its process handle which exits immediately.  The second
process then starts and fails to connect to the Mojo channel.

I investigated whether UiAccess is truly required for the desktop binary
and I think that it is.  For the Ctrl+Alt+Del scenario, there are registry
keys that can be set which will require that flag.  For Alt+Tab, it is
possible that some windows might not be accessible if they have a high
high enough integrity level (+ UiAccess themselves).

So instead of removing the UiAccess flag, my approach is to listen for the
worker process creation and exit events.  I store the value of the last seen
worker process id and use that in our process launch detection code once the
launcher process exits.  This allows both un-official builds (which do not
require the extra permissions hop) and official builds will work consistently.

BUG= 666992 

Review-Url: https://codereview.chromium.org/2568983004
Cr-Commit-Position: refs/heads/master@{#438077}

[modify] https://crrev.com/1cdb04810de9689ba0ea5ab7e70bd35ce5477063/remoting/host/desktop_session_win.cc
[modify] https://crrev.com/1cdb04810de9689ba0ea5ab7e70bd35ce5477063/remoting/host/win/BUILD.gn
[modify] https://crrev.com/1cdb04810de9689ba0ea5ab7e70bd35ce5477063/remoting/host/win/wts_session_process_delegate.cc

Labels: -Hotlist-Merge-Approved -merge-merged-2924 Merge-Request-56
After some additional investigation, I have an additional fix I would like to apply to M56 for this problem.  The first fix unblocked our tester to look for additional problems, however this second fix is the right change to release.

Removing previous labels and requesting merge for M56.

Comment 13 by dimu@chromium.org, Dec 14 2016

Labels: -Merge-Request-56 Merge-Approved-56 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M56 (branch: 2924)
Project Member

Comment 14 by sheriffbot@chromium.org, Dec 14 2016

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 15 by bugdroid1@chromium.org, Dec 14 2016

Labels: -merge-approved-56 merge-merged-2924
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/286e4172c0f1d07fa50d101ff46d20ad42aefb3e

commit 286e4172c0f1d07fa50d101ff46d20ad42aefb3e
Author: Joe Downing <joedow@google.com>
Date: Wed Dec 14 16:58:33 2016

Fixing CRD curtain mode connection failures on Win8+ official builds

I've root caused this and the problem was introduced in this CL:
https://codereview.chromium.org/2446053002

That CL moved the logic which reported the worker process launch from the
IPC Channel Connected method to an IO completion port listener.  This works
fine on un-official builds, however it breaks on official builds.  The
reason for the failure is that the remoting_desktop process on official
builds are signed and include 'UiAccess' in their manifest.  When the
launcher process uses ShellExecuteEx to launch the worker process in this
scenario, we actually see two processes get launched sequentially.  The
first starts and exists with error code 'STATUS_ELEVATION_REQUIRED' and
the second launches with the correct permissions.  This behavior worked
fine before as we listened for the connection to the IPC channel which
was done by the second, successful process launch.  With the new code,
we observed the first process launch, set up the Mojo channel for it, and
tried waiting on its process handle which exits immediately.  The second
process then starts and fails to connect to the Mojo channel.

I investigated whether UiAccess is truly required for the desktop binary
and I think that it is.  For the Ctrl+Alt+Del scenario, there are registry
keys that can be set which will require that flag.  For Alt+Tab, it is
possible that some windows might not be accessible if they have a high
high enough integrity level (+ UiAccess themselves).

So instead of removing the UiAccess flag, my approach is to listen for the
worker process creation and exit events.  I store the value of the last seen
worker process id and use that in our process launch detection code once the
launcher process exits.  This allows both un-official builds (which do not
require the extra permissions hop) and official builds will work consistently.

BUG= 666992 

Review-Url: https://codereview.chromium.org/2568983004
Cr-Commit-Position: refs/heads/master@{#438077}
(cherry picked from commit 1cdb04810de9689ba0ea5ab7e70bd35ce5477063)

Review-Url: https://codereview.chromium.org/2575103002 .
Cr-Commit-Position: refs/branch-heads/2924@{#490}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}

[modify] https://crrev.com/286e4172c0f1d07fa50d101ff46d20ad42aefb3e/remoting/host/desktop_session_win.cc
[modify] https://crrev.com/286e4172c0f1d07fa50d101ff46d20ad42aefb3e/remoting/host/win/BUILD.gn
[modify] https://crrev.com/286e4172c0f1d07fa50d101ff46d20ad42aefb3e/remoting/host/win/wts_session_process_delegate.cc

Status: Fixed (was: Started)
AJ, this change should now exist in both M56 and M57 as of tonight's build.

Can you take a look at Win7 and Win10 and verify it is working as expected now? 
Owner: ajnolley@chromium.org
Status: Verified (was: Fixed)
Verified Fixed in 57.0.2952.0 on Windows 10. Looks good on Windows 7 as well.

Sign in to add a comment