Fixes to possible GetLastError bugs

Fixes to possible GetLastError bugs

GetLastError needs to be called immediately after the function whose
error code it is retrieving. This has been made particularly important
by VS 2015 which sometimes clears the error code when allocating memory.
See bug 528394 for details of the underlying issue.

These changes came from code inspection that looked for patterns that
appeared wrong. None of the issues look critical, although that depends
on what callers do with the error codes.

BUG=529981
Committed: https://crrev.com/4af09d1d3452796370999fcb6add64884257e80b
Cr-Commit-Position: refs/heads/master@{#349481}

[brucedawson, 2015-09-12 00:10:55]

brucedawson@chromium.org changed reviewers:
+ cpu@chromium.org, rvargas@chromium.org

[brucedawson, 2015-09-12 00:33:56]

After being burned by one bug with calling GetLastError() too late
I searched most of Chrome for more instances. Other than a bunch
of possible logging errors (not addressed) I just found a few
possibly harmless errors, fixed here.

rvargas@ 'cause he reviewed the related fix, and cpu 'cause of the sandbox code
(and password_manager, apparently).

https://codereview.chromium.org/1337223002/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/password_manager_util_win.cc (right):

https://codereview.chromium.org/1337223002/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/password_manager_util_win.cc:132: auto
last_error = GetLastError();
CloseHandle could affect LastError. Unlikely, but possible.

https://codereview.chromium.org/1337223002/diff/1/sandbox/win/src/process_thr...
File sandbox/win/src/process_thread_interception.cc (right):

https://codereview.chromium.org/1337223002/diff/1/sandbox/win/src/process_thr...
sandbox/win/src/process_thread_interception.cc:281: if
(SandboxFactory::GetTargetServices()->GetState()->InitCalled()) {
Moving this below the initialization of original_error because I can't tell
whether it calls anything that could perturb LastError.

https://codereview.chromium.org/1337223002/diff/1/sandbox/win/src/restricted_...
File sandbox/win/src/restricted_token.cc (right):

https://codereview.chromium.org/1337223002/diff/1/sandbox/win/src/restricted_...
sandbox/win/src/restricted_token.cc:143: auto last_error = ::GetLastError();
VS 2015 makes "operator new" clear LastError in some build configurations.
"operator delete" might also.

https://codereview.chromium.org/1337223002/diff/1/sandbox/win/src/restricted_...
File sandbox/win/src/restricted_token_utils.cc (right):

https://codereview.chromium.org/1337223002/diff/1/sandbox/win/src/restricted_...
sandbox/win/src/restricted_token_utils.cc:221: auto last_error =
::GetLastError();
Latch LastError before LocalFree. Probably doesn't clear LastError on success,
but it isn't specified.

https://codereview.chromium.org/1337223002/diff/1/sandbox/win/tools/launcher/...
File sandbox/win/tools/launcher/launcher.cc (right):

https://codereview.chromium.org/1337223002/diff/1/sandbox/win/tools/launcher/...
sandbox/win/tools/launcher/launcher.cc:105: auto last_error = ::GetLastError();
I assume that we want the SetThreadToken error, not the TerminateProcess error.
Same below.

[vabr (Chromium), 2015-09-13 11:31:01]

vabr@chromium.org changed reviewers:
+ vabr@chromium.org

[vabr (Chromium), 2015-09-13 11:31:02]

chrome/browser/password_manager/password_manager_util_win.cc LGTM, thanks for
checking for this issue in other parts of Chromium!
Cheers,
Vaclav

[cpu_(ooo_6.6-7.5), 2015-09-14 17:17:16]

https://codereview.chromium.org/1337223002/diff/1/sandbox/win/src/process_thr...
File sandbox/win/src/process_thread_interception.cc (right):

https://codereview.chromium.org/1337223002/diff/1/sandbox/win/src/process_thr...
sandbox/win/src/process_thread_interception.cc:281: if
(SandboxFactory::GetTargetServices()->GetState()->InitCalled()) {
On 2015/09/12 00:33:55, brucedawson wrote:
> Moving this below the initialization of original_error because I can't tell
> whether it calls anything that could perturb LastError.

In this particular case it is harmless to move it but in general we should keep
the interception code as it was. Reason being we can reach a TargetXXX function
before anything but ntdll has properly appeared in our process, including
kernel32.

So the answer is InitCalled() is just a memory check, so lets leave as it was.

[brucedawson, 2015-09-14 18:59:55]

PTAL

In process_thread_interception.cc could add a comment (current CL) or do
nothing.

https://codereview.chromium.org/1337223002/diff/1/sandbox/win/src/process_thr...
File sandbox/win/src/process_thread_interception.cc (right):

https://codereview.chromium.org/1337223002/diff/1/sandbox/win/src/process_thr...
sandbox/win/src/process_thread_interception.cc:281: if
(SandboxFactory::GetTargetServices()->GetState()->InitCalled()) {
Got it.

How about a comment to make the rational clearer. Possible comment added.

[rvargas (doing something else), 2015-09-14 19:30:57]

base & sbox lgtm

[cpu_(ooo_6.6-7.5), 2015-09-17 19:09:55]

lgtm

[brucedawson, 2015-09-17 19:40:12]

The CQ bit was checked by brucedawson@chromium.org

[brucedawson, 2015-09-17 19:40:13]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/1337223002/#ps20001
(title: "Removed some sandbox changes")

[commit-bot: I haz the power, 2015-09-17 19:40:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1337223002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1337223002/20001

[commit-bot: I haz the power, 2015-09-17 20:21:52]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-09-17 20:22:27]

Patchset 2 (id:??) landed as
https://crrev.com/4af09d1d3452796370999fcb6add64884257e80b
Cr-Commit-Position: refs/heads/master@{#349481}