Closed Bug 453185 Opened 12 years ago Closed 12 years ago

nsIProcess run fails with NS_ERROR_FILE_EXECUTION_FAILED when running an exe with a manifest that specifies requestedExecutionLevel level="requireAdministrator" on Vista

Categories

(Core :: XPCOM, defect)

x86
Windows Vista
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: robert.strong.bugs, Assigned: mossop)

References

Details

(Keywords: fixed1.9.1)

Attachments

(3 files, 1 obsolete file)

This is due to nsIProcess using CreateProcess where the app has a manifest with requestedExecutionLevel level="asInvoker" and the system has UAC turned on. nsIProcess should likely use ShellExecuteEx
Status: UNCONFIRMED → NEW
Ever confirmed: true
script I've been using for testing... change path for the test exe to where ever you saved the exe. You can also specify an exe that doesn't have a manifest that specifies requestedExecutionLevel level="requireAdministrator" such as notepad to verify that it does work for the other cases

var exePath = "C:\\test.exe";
var process = Components.classes["@mozilla.org/process/util;1"].
                createInstance(Components.interfaces.nsIProcess);
var file = Components.classes["@mozilla.org/file/local;1"].
             createInstance(Components.interfaces.nsILocalFile);
file.initWithPath(exePath);
file.QueryInterface(Components.interfaces.nsIFile);
try {
  process.init(file);
  process.run(true, [], 0);
} catch (e) {
  alert("bleh " + e + "\n\n");
}
Attached patch patch rev1 - use ShellExecuteEx (obsolete) — Splinter Review
Since the scenario that launches the binary will bring up the UAC dialog on a Vista system there is no way I know of to test this... if there were we would have likely found a major security hole in the UAC implementation. Are there any tests you think should be added that can be implemented?
Assignee: nobody → robert.bugzilla
Status: NEW → ASSIGNED
Attachment #336377 - Flags: review?(benjamin)
Attachment #336377 - Flags: review?(jmathies)
Comment on attachment 336377 [details] [diff] [review]
patch rev1 - use ShellExecuteEx

Jim, could you also take a look at this? Thanks!
Comment on attachment 336377 [details] [diff] [review]
patch rev1 - use ShellExecuteEx

>diff --git a/xpcom/threads/nsProcessCommon.cpp b/xpcom/threads/nsProcessCommon.cpp
>--- a/xpcom/threads/nsProcessCommon.cpp
>+++ b/xpcom/threads/nsProcessCommon.cpp
>...
>+    // Sets hProcess to the process handle.
>+    if (blocking)
>+        sinfo.fMask += SEE_MASK_NOCLOSEPROCESS;
should be |=
Looks good to me, pretty heavy change though to a key chunk of code. I assume this will be baking for a while?
Attachment #336377 - Flags: review?(jmathies) → review+
(In reply to comment #6)
> Looks good to me, pretty heavy change though to a key chunk of code. I assume
> this will be baking for a while?
Most likely
Comment on attachment 336377 [details] [diff] [review]
patch rev1 - use ShellExecuteEx

This looks good, though I desperately want some tests...
Attachment #336377 - Flags: review?(benjamin) → review+
(In reply to comment #8)
> (From update of attachment 336377 [details] [diff] [review])
> This looks good, though I desperately want some tests...
We can't test the UAC from what I've been able to find. For what it is worth there is a test I landed yesterday (bug 451085) that uses nsIProcess so it at lease gets tested elsewhere for the general case where elevation isn't required.
Blocks: 435788
Cleaning this patch up to work with the current state of nsIProcess...
Assignee: robert.bugzilla → dtownsend
Attached patch patch rev 2Splinter Review
This patch applies on top of the patch in bug 442393.

This is mostly the same stuff but has had to undergo some rearrangement to cope with changes introduced by other patches. It switches to using ShellExecuteEx to launch the process and just keeps a hold of the process HANDLE that comes back. This allows us to kill etc. the process when run non-blocking. It also adds support for getting the exit value of a process that was run in a non-blocking manner. This only works on windows though, I can't see a way to make it work elsewhere.

There is one problem, ShellExecuteEx doesn't give us the PID of the process. The API call to get a PID from a process HANDLE is only available on XP SP1 and later. I've implemented it such that it works there and later. If we wanted getting the PID to work everywhere then we might be able to cope by trying CreateProcess first then using ShellExecuteEx if that doesn't work. However I don't think being able to get the PID is such a critical need that we should complicate things like that for now. Getting the PID was only implemented a short time ago in bug 442393 so we aren't really regressing anything in a release.

As Rob says there doesn't seem to be a way that we can test the UAC response to this automatically. We do now have a few different tests of nsIProcess that this all passes which I'm hoping will be enough to allow checking this in this time. I have verified that the UAC does appear using the testcase in this bug report on Windows 7, both running blocking and non-blocking, and in both cases we get an appropriate exitValue (0 or 2 depending on if you click yes or no in the dialog). For both blocking and non-blocking we still throw NS_ERROR_FILE_EXECUTION_FAILED out of the run call if the user denies the UAC request.

I don't have access to a Vista box to verify it works for sure there, Rob maybe you might be able to give it a spin for me?
Attachment #336377 - Attachment is obsolete: true
Attachment #367797 - Flags: review?(benjamin)
Comment on attachment 367797 [details] [diff] [review]
patch rev 2

>diff --git a/xpcom/threads/nsProcessCommon.cpp b/xpcom/threads/nsProcessCommon.cpp

>+    HMODULE kernelDLL = ::LoadLibraryW(L"Kernel32.dll");

I slightly prefer all-lowercase here.
Attachment #367797 - Flags: review?(benjamin) → review+
Pushed: http://hg.mozilla.org/mozilla-central/rev/1f3d5e332a44
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
This push busted windows ce, this patch gets us building
Attachment #368035 - Flags: review?(benjamin)
Attachment #368035 - Flags: review?(benjamin) → review+
Comment on attachment 367797 [details] [diff] [review]
patch rev 2

We need to take this on 1.9.1 if we want to be able to use nsIProcess to run installers that require elevation on Vista. The existing nsIProcess tests verify that it does not break anything, but we are unable to use automated testing to show that elevation works, however that has be tested manually.
Attachment #367797 - Flags: approval1.9.1?
Attachment #367797 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 367797 [details] [diff] [review]
patch rev 2

a191=beltzner for this and the WINCE fix
Landed with the bustage fix on branch: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/726d6e0d61de
Keywords: fixed1.9.1
Depends on: 484747
Depends on: 485946
No longer depends on: 484747
You need to log in before you can comment on or make changes to this bug.