Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1273183002:
Fix invalid C-style cast in RenderFrameImplTest. (Closed)
|
Created:
5 years, 4 months ago by krasin Modified:
5 years, 4 months ago Reviewers:
nasko CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix invalid C-style cast in RenderFrameImplTest.
It casts (like reinterpret_cast) from one unrelated type
(content::RenderView*) to another unrelated type
(content::RenderWidget*). The result pointer is invalid,
but it's only used in EXPECT_NE, which would always be satisfied.
Using the correct static cast instead.
The bug was found by Control Flow Integrity check:
https://sites.google.com/a/chromium.org/dev/developers/testing/control-flow-integrity
BUG=chromium:457523, chromium:517544
Committed: https://crrev.com/25646da121a85f51e54f52c161172d7e5c614aea
Cr-Commit-Position: refs/heads/master@{#342222}
Patch Set 1 #
Total comments: 1
Patch Set 2 : two casts; comments #
Total comments: 6
Patch Set 3 : nits #
Total comments: 1
Patch Set 4 : remove two more content:: #Messages
Total messages: 24 (6 generated)
krasin@google.com changed reviewers: + nasko@chromium.org
https://codereview.chromium.org/1273183002/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl_browsertest.cc (right): https://codereview.chromium.org/1273183002/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl_browsertest.cc:114: static_cast<content::RenderViewImpl*>((view_))); This is not a correct cast. The subframe should have a RenderWidget, which is not a RenderViewImpl. Also, the view_ as a RenderWidget should be different than what frame_widget() returns, since they are different instances.
On 2015/08/06 18:08:33, nasko wrote: > https://codereview.chromium.org/1273183002/diff/1/content/renderer/render_fra... > File content/renderer/render_frame_impl_browsertest.cc (right): > > https://codereview.chromium.org/1273183002/diff/1/content/renderer/render_fra... > content/renderer/render_frame_impl_browsertest.cc:114: > static_cast<content::RenderViewImpl*>((view_))); > This is not a correct cast. The subframe should have a RenderWidget, which is > not a RenderViewImpl. Also, the view_ as a RenderWidget should be different than > what frame_widget() returns, since they are different instances. Sorry, there's a chance that I misunderstand what you're saying. To the best of my (and CFI check) knowledge the cast is technically correct. Are you saying this EXPECT_NE should have a different argument? If yes, do you have any suggestions? The real type of view_ is RenderViewImpl, so I don't know how to make it not RenderViewImpl.
On 2015/08/06 18:42:39, krasin wrote: > On 2015/08/06 18:08:33, nasko wrote: > > > https://codereview.chromium.org/1273183002/diff/1/content/renderer/render_fra... > > File content/renderer/render_frame_impl_browsertest.cc (right): > > > > > https://codereview.chromium.org/1273183002/diff/1/content/renderer/render_fra... > > content/renderer/render_frame_impl_browsertest.cc:114: > > static_cast<content::RenderViewImpl*>((view_))); > > This is not a correct cast. The subframe should have a RenderWidget, which is > > not a RenderViewImpl. Also, the view_ as a RenderWidget should be different > than > > what frame_widget() returns, since they are different instances. > > Sorry, there's a chance that I misunderstand what you're saying. > > To the best of my (and CFI check) knowledge the cast is technically correct. Your new cast is technically correct. > Are you saying this EXPECT_NE should have a different argument? If yes, do you > have any suggestions? No, the arguments are the correct ones, but the casts aren't. > The real type of view_ is RenderViewImpl, so I don't know how to make it not > RenderViewImpl. If you look at the inheritance, RenderViewImpl is also RenderWidget, as it inherits from it (https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...). Therefore view_ can be cast to RenderWidget. Since frame_widget() returns RenderWidget*, it makes sense to cast view_ to RenderWidget*, so we are comparing the same types, which is what I was trying to say initially.
> If you look at the inheritance, RenderViewImpl is also RenderWidget, as it > inherits from it > (https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...). > Therefore view_ can be cast to RenderWidget. Since frame_widget() returns > RenderWidget*, it makes sense to cast view_ to RenderWidget*, so we are > comparing the same types, which is what I was trying to say initially. Ah! Now, I understand you. It seems that I had the same idea yesterday, and then it was explained to me why such direct cast is invalid: https://code.google.com/p/chromium/issues/detail?id=517262#c3 Peter Collingbourne said: "If a class has multiple base classes, each base class pointer will be distinct, and direct casts between unrelated base classes are undefined, even if they share a common derived class. The compiler has no way in general of determining the displacement between the two classes, so these types of casts cannot be implemented in a type safe way. In this case, because the code uses a C-style cast, the cast acts like a reinterpret_cast." As for comparing the same types, will the following work? static_cast<RenderWidget*>(static_cast<RenderViewImpl*>(view_))
> As for comparing the same types, will the following work? > > static_cast<RenderWidget*>(static_cast<RenderViewImpl*>(view_)) By that I mean, will it look good to you; technically, these casts work, I checked that.
On 2015/08/06 19:36:55, krasin wrote: > > As for comparing the same types, will the following work? > > > > static_cast<RenderWidget*>(static_cast<RenderViewImpl*>(view_)) > By that I mean, will it look good to you; technically, these casts work, I > checked that. I think this will work, but it will be good to include (at least part of) the explanation you gave me, as I didn't know the details of it.
On 2015/08/06 20:14:21, nasko wrote: > On 2015/08/06 19:36:55, krasin wrote: > > > As for comparing the same types, will the following work? > > > > > > static_cast<RenderWidget*>(static_cast<RenderViewImpl*>(view_)) > > By that I mean, will it look good to you; technically, these casts work, I > > checked that. > > I think this will work, but it will be good to include (at least part of) the > explanation you gave me, as I didn't know the details of it. Will do. I agree, the comments are needed. This is an edge case, which I didn't know about despite using C++ all these years.
I have added the comments, and the second cast to RenderWidget*. Please, take another look!
LGTM with nits. https://codereview.chromium.org/1273183002/diff/20001/content/renderer/render... File content/renderer/render_frame_impl_browsertest.cc (right): https://codereview.chromium.org/1273183002/diff/20001/content/renderer/render... content/renderer/render_frame_impl_browsertest.cc:114: // because it and content::RenderView are two unrelated base classes nit: drop the "content::" prefix, as this code is already in the content namespace. https://codereview.chromium.org/1273183002/diff/20001/content/renderer/render... content/renderer/render_frame_impl_browsertest.cc:121: // So overcome this, we make two legal static casts: nit: s/So/To/ https://codereview.chromium.org/1273183002/diff/20001/content/renderer/render... content/renderer/render_frame_impl_browsertest.cc:123: // then upcast from RenderViewImpl* to RenderWidger*. nit: RenderWidget
https://codereview.chromium.org/1273183002/diff/20001/content/renderer/render... File content/renderer/render_frame_impl_browsertest.cc (right): https://codereview.chromium.org/1273183002/diff/20001/content/renderer/render... content/renderer/render_frame_impl_browsertest.cc:114: // because it and content::RenderView are two unrelated base classes On 2015/08/06 20:55:20, nasko wrote: > nit: drop the "content::" prefix, as this code is already in the content > namespace. Done. https://codereview.chromium.org/1273183002/diff/20001/content/renderer/render... content/renderer/render_frame_impl_browsertest.cc:121: // So overcome this, we make two legal static casts: On 2015/08/06 20:55:20, nasko wrote: > nit: s/So/To/ Done. https://codereview.chromium.org/1273183002/diff/20001/content/renderer/render... content/renderer/render_frame_impl_browsertest.cc:123: // then upcast from RenderViewImpl* to RenderWidger*. On 2015/08/06 20:55:20, nasko wrote: > nit: RenderWidget Done.
The CQ bit was checked by krasin@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org Link to the patchset: https://codereview.chromium.org/1273183002/#ps40001 (title: "nits")
Thank you for the review and sorry for a little bit of confusion in the beginning. I ought to include the comment to the original revision of the CL.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1273183002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1273183002/40001
The CQ bit was unchecked by nasko@chromium.org
https://codereview.chromium.org/1273183002/diff/40001/content/renderer/render... File content/renderer/render_frame_impl_browsertest.cc (right): https://codereview.chromium.org/1273183002/diff/40001/content/renderer/render... content/renderer/render_frame_impl_browsertest.cc:114: // it and content::RenderView are two unrelated base classes The content:: prefixes are still in the comments. Please remove all of the occurences.
On 2015/08/06 21:05:30, nasko wrote: > https://codereview.chromium.org/1273183002/diff/40001/content/renderer/render... > File content/renderer/render_frame_impl_browsertest.cc (right): > > https://codereview.chromium.org/1273183002/diff/40001/content/renderer/render... > content/renderer/render_frame_impl_browsertest.cc:114: // it and > content::RenderView are two unrelated base classes > The content:: prefixes are still in the comments. Please remove all of the > occurences. Done
The CQ bit was checked by krasin@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org Link to the patchset: https://codereview.chromium.org/1273183002/#ps60001 (title: "remove two more content::")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1273183002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1273183002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/25646da121a85f51e54f52c161172d7e5c614aea Cr-Commit-Position: refs/heads/master@{#342222} |
