Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2004423007:
SourceLocation: Fix use-after-move. (Closed)
DescriptionSourceLocation: 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.)
Committed: https://crrev.com/9a1916358318e35f8045864784dc757bfb617eee
Cr-Commit-Position: refs/heads/master@{#396154}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Reflect review comments. #
Messages
Total messages: 27 (8 generated)
Description was changed from ========== Fix crash in many pages. Depending on the arguments evaluation order, the previous code may access std::move'ed object. BUG=614900 TEST=Manually verified new tab page doesn't crash (which previously did.) ========== to ========== 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.) ==========
kinaba@chromium.org changed reviewers: + bashi@chromium.org, dgozman@chromium.org
Meant to be a P0-bug fixer. bashi-san, could you take a look as a third_party/WebKit/Source/bindings OWNER?
yukishiino@chromium.org changed reviewers: + yukishiino@chromium.org
Thanks for finding out the issue. https://codereview.chromium.org/2004423007/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/SourceLocation.cpp (right): https://codereview.chromium.org/2004423007/diff/1/third_party/WebKit/Source/b... 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/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/SourceLocation.cpp (right): https://codereview.chromium.org/2004423007/diff/1/third_party/WebKit/Source/b... 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.
Thanks! How about this one? https://codereview.chromium.org/2004423007/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/SourceLocation.cpp (right): https://codereview.chromium.org/2004423007/diff/1/third_party/WebKit/Source/b... 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/b... 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!
Thanks for fixing the issue. LGTM.
On 2016/05/26 07:17:43, Yuki wrote: > Thanks for fixing the issue. LGTM. Thanks for you too for prompt review!
The CQ bit was checked by kinaba@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
LGTM
lgtm
haraken@chromium.org changed reviewers: + haraken@chromium.org, yutak@chromium.org
+yutak FYI We need to be careful about this pattern. (In essence, OwnPtr.release() should have had the same issue though.)
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...
yutak@chromium.org changed reviewers: + thakis@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.
Message was sent while issue was closed.
Description was changed from ========== 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.) ========== to ========== 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.) ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== 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.) ========== to ========== 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.) Committed: https://crrev.com/9a1916358318e35f8045864784dc757bfb617eee Cr-Commit-Position: refs/heads/master@{#396154} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/9a1916358318e35f8045864784dc757bfb617eee Cr-Commit-Position: refs/heads/master@{#396154}
Message was sent while issue was closed.
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?
Message was sent while issue was closed.
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).
Message was sent while issue was closed.
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.
Message was sent while issue was closed.
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).
Message was sent while issue was closed.
(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.) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
