SourceLocation: Fix use-after-move. (issue 2004423007 by kinaba@chromium.org)

[kin...@chromium.org]

Reviewers: bashi1, dgozman
CL: https://codereview.chromium.org/2004423007/

Message:
Meant to be a P0-bug fixer.

bashi-san, could you take a look as a third_party/WebKit/Source/bindings OWNER?

Description:
SourceLocation: Fix use-after-move.

Depending on the arguments evaluation order, the previous code may access
std::move'ed object. It is triggering crashes in many pages
including google.com on Chrome OS (built by gcc).

BUG=614900
TEST=Manually verified new tab page doesn't crash (which previously did.)

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+8, -4 lines):
M third_party/WebKit/Source/bindings/core/v8/SourceLocation.cpp


Index: third_party/WebKit/Source/bindings/core/v8/SourceLocation.cpp
diff --git a/third_party/WebKit/Source/bindings/core/v8/SourceLocation.cpp b/third_party/WebKit/Source/bindings/core/v8/SourceLocation.cpp
index f7e4197f4b94008753555589c481ea87de5ecacc..c806cf2a0ab609b14bf1862d1a30a82c0134ecea 100644
--- a/third_party/WebKit/Source/bindings/core/v8/SourceLocation.cpp
+++ b/third_party/WebKit/Source/bindings/core/v8/SourceLocation.cpp
@@ -41,8 +41,10 @@ std::unique_ptr<V8StackTrace> captureStackTrace()
PassOwnPtr<SourceLocation> SourceLocation::capture(const String& url, unsigned lineNumber, unsigned columnNumber)
{
std::unique_ptr<V8StackTrace> stackTrace = captureStackTrace();
- if (stackTrace && !stackTrace->isEmpty())
- return SourceLocation::create(stackTrace->topSourceURL(), stackTrace->topLineNumber(), stackTrace->topColumnNumber(), std::move(stackTrace), 0);
+ if (stackTrace && !stackTrace->isEmpty()) {
+ const V8StackTrace* stackTracePtr = stackTrace.get();
+ return SourceLocation::create(stackTracePtr->topSourceURL(), stackTracePtr->topLineNumber(), stackTracePtr->topColumnNumber(), std::move(stackTrace), 0);
+ }
return SourceLocation::create(url, lineNumber, columnNumber, std::move(stackTrace));
}

@@ -50,8 +52,10 @@ PassOwnPtr<SourceLocation> SourceLocation::capture(const String& url, unsigned l
PassOwnPtr<SourceLocation> SourceLocation::capture(ExecutionContext* executionContext)
{
std::unique_ptr<V8StackTrace> stackTrace = captureStackTrace();
- if (stackTrace && !stackTrace->isEmpty())
- return SourceLocation::create(stackTrace->topSourceURL(), stackTrace->topLineNumber(), stackTrace->topColumnNumber(), std::move(stackTrace), 0);
+ if (stackTrace && !stackTrace->isEmpty()) {
+ const V8StackTrace* stackTracePtr = stackTrace.get();
+ return SourceLocation::create(stackTracePtr->topSourceURL(), stackTracePtr->topLineNumber(), stackTracePtr->topColumnNumber(), std::move(stackTrace), 0);
+ }

Document* document = executionContext && executionContext->isDocument() ? toDocument(executionContext) : nullptr;
if (document) {

[yukis...@chromium.org]

Thanks for finding out the issue.


https://codereview.chromium.org/2004423007/diff/1/third_party/WebKit/Source/bindings/core/v8/SourceLocation.cpp
File third_party/WebKit/Source/bindings/core/v8/SourceLocation.cpp
(right):

https://codereview.chromium.org/2004423007/diff/1/third_party/WebKit/Source/bindings/core/v8/SourceLocation.cpp#newcode45
third_party/WebKit/Source/bindings/core/v8/SourceLocation.cpp:45: const
V8StackTrace* stackTracePtr = stackTrace.get();
I'm a bit uneasy to use a raw pointer here.
const String& url = stackTrace->topSourceURL();
const unsigned lineNumber = stackTrace->topLineNumber();
...
return SourceLocation::create(url, lineNumber, ...);
seems more "right things" to me, personally.

Anyway, without a comment, it's not easy to understand the intention.
I'd propose the following things in the preferred order.

1) Adds SourceLocation::create(std::unique_ptr<V8StackTrace>, int
scriptId = 0) and use it. No need to pass url, line number, etc. seeing
these two cases.
2) Adds a comment to explain the fix. Why it's needed.
2-1) [optional] Use temporary variables rather than a raw pointer.

It's fine to assign the issue to me if you don't have time.

https://codereview.chromium.org/2004423007/

[yukis...@chromium.org]

https://codereview.chromium.org/2004423007/diff/1/third_party/WebKit/Source/bindings/core/v8/SourceLocation.cpp
File third_party/WebKit/Source/bindings/core/v8/SourceLocation.cpp
(right):

https://codereview.chromium.org/2004423007/diff/1/third_party/WebKit/Source/bindings/core/v8/SourceLocation.cpp#newcode96
third_party/WebKit/Source/bindings/core/v8/SourceLocation.cpp:96: return
adoptPtr(new SourceLocation(stackTrace->topSourceURL(),
stackTrace->topLineNumber(), stackTrace->topColumnNumber(),
std::move(stackTrace), 0));
This line also seems problematic.

https://codereview.chromium.org/2004423007/

[kin...@chromium.org]

Thanks! How about this one?

https://codereview.chromium.org/2004423007/diff/1/third_party/WebKit/Source/bindings/core/v8/SourceLocation.cpp
File third_party/WebKit/Source/bindings/core/v8/SourceLocation.cpp
(right):

https://codereview.chromium.org/2004423007/diff/1/third_party/WebKit/Source/bindings/core/v8/SourceLocation.cpp#newcode45
third_party/WebKit/Source/bindings/core/v8/SourceLocation.cpp:45: const
V8StackTrace* stackTracePtr = stackTrace.get();

On 2016/05/26 06:55:50, Yuki wrote:
> I'm a bit uneasy to use a raw pointer here.
> const String& url = stackTrace->topSourceURL();
> const unsigned lineNumber = stackTrace->topLineNumber();
> ...
> return SourceLocation::create(url, lineNumber, ...);
> seems more "right things" to me, personally.
>
> Anyway, without a comment, it's not easy to understand the intention.
I'd
> propose the following things in the preferred order.
>
> 1) Adds SourceLocation::create(std::unique_ptr<V8StackTrace>, int
scriptId = 0)
> and use it. No need to pass url, line number, etc. seeing these two
cases.
> 2) Adds a comment to explain the fix. Why it's needed.
> 2-1) [optional] Use temporary variables rather than a raw pointer.
>
> It's fine to assign the issue to me if you don't have time.

Makes sense. Done.

https://codereview.chromium.org/2004423007/diff/1/third_party/WebKit/Source/bindings/core/v8/SourceLocation.cpp#newcode96
third_party/WebKit/Source/bindings/core/v8/SourceLocation.cpp:96: return
adoptPtr(new SourceLocation(stackTrace->topSourceURL(),
stackTrace->topLineNumber(), stackTrace->topColumnNumber(),
std::move(stackTrace), 0));

On 2016/05/26 07:05:36, Yuki wrote:
> This line also seems problematic.

Good catch!

https://codereview.chromium.org/2004423007/

[yukis...@chromium.org]

Thanks for fixing the issue. LGTM.

https://codereview.chromium.org/2004423007/

[kin...@chromium.org]

On 2016/05/26 07:17:43, Yuki wrote:
> Thanks for fixing the issue. LGTM.

Thanks for you too for prompt review!

https://codereview.chromium.org/2004423007/

[commit-bot@chromium.org via codereview.chromium.org]

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


https://codereview.chromium.org/2004423007/

[har...@chromium.org]

LGTM


https://codereview.chromium.org/2004423007/

[ba...@chromium.org]

lgtm

https://codereview.chromium.org/2004423007/

[har...@chromium.org]

+yutak FYI

We need to be careful about this pattern. (In essence, OwnPtr.release() should
have had the same issue though.)



https://codereview.chromium.org/2004423007/

[kin...@chromium.org]

Thank you all!!

On 2016/05/26 07:36:50, haraken wrote:
> +yutak FYI
>
> We need to be careful about this pattern. (In essence, OwnPtr.release() should
> have had the same issue though.)

It may be a good idea to have clang plugin for detecting this pattern.
I remember I fixed this kind of bug 4 or 5 times in Chromium...

https://codereview.chromium.org/2004423007/

[yu...@chromium.org]

+thakis

On 2016/05/26 07:40:10, kinaba wrote:
> It may be a good idea to have clang plugin for detecting this pattern.
> I remember I fixed this kind of bug 4 or 5 times in Chromium...

Or, this kind of warning may even be implemented in Clang proper,
because the issue is not limited to OwnPtr; this basically
applies to any function call with an argument invoking a non-const
member function of some object and with another argument containing
the use of that object.

https://codereview.chromium.org/2004423007/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #2 (id:20001)

https://codereview.chromium.org/2004423007/

[commit-bot@chromium.org via codereview.chromium.org]

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

https://codereview.chromium.org/2004423007/

[dgo...@chromium.org]

Sorry for breakage. Thanks for the fix!

I'm surprised this fails though, as I was careful to put movable argument last
so it would be possible to access it. Isn't compiler supposed to evaluate
parameters left-to-right?

https://codereview.chromium.org/2004423007/

[tha...@chromium.org]

A compiler warning for this is
https://bugs.chromium.org/p/chromium/issues/detail?id=409318#c16 -- there's even
a prototype patch there

On 2016/05/26 14:12:18, dgozman wrote:
> Sorry for breakage. Thanks for the fix!
>
> I'm surprised this fails though, as I was careful to put movable argument last
> so it would be possible to access it. Isn't compiler supposed to evaluate
> parameters left-to-right?

No, that's implementation-defined (and gcc picks a different order than MSVC).

https://codereview.chromium.org/2004423007/

[kin...@chromium.org]

On 2016/05/26 14:12:18, dgozman wrote:
> Sorry for breakage. Thanks for the fix!
>
> I'm surprised this fails though, as I was careful to put movable argument last
> so it would be possible to access it. Isn't compiler supposed to evaluate
> parameters left-to-right?

Evaluation order is not defined in C++ spec (at least as of C++14).
Clang looks leaning toward keeping left-to-right order,
but others (e.g., gcc) indeed take advantage of freedom.
It quite often does it right-to-left on x86.

https://codereview.chromium.org/2004423007/

[yukis...@chromium.org]

On 2016/05/26 14:12:18, dgozman wrote:
> Sorry for breakage. Thanks for the fix!
>
> I'm surprised this fails though, as I was careful to put movable argument last
> so it would be possible to access it. Isn't compiler supposed to evaluate
> parameters left-to-right?

Unlike Java, it's an unspecified behavior in C or C++. Compilers are allowed to
evaluate arguments in an arbitrary order (event not left-to-right nor
right-to-left).

https://codereview.chromium.org/2004423007/

[tha...@chromium.org]

(fwiw it looks like the clang/win bots also caught this,
https://bugs.chromium.org/p/chromium/issues/detail?id=614745 But the clang/win
bot on the CQ doesn't run tests, and we didn't get around to analyzing this
yesterday, sorry.)

https://codereview.chromium.org/2004423007/