Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2031763003:
Remove itself from the widget observers in destructor (Closed)
DescriptionRemove itself from the widget observers in destructor
This patch may fix the use-after-free reported in crbug.com/612050.
BUG=612050
TEST=msan passes
Committed: https://crrev.com/58f821b32a3b99bf15855225dff13d77f10d7484
Cr-Commit-Position: refs/heads/master@{#402408}
Patch Set 1 #
Total comments: 3
Patch Set 2 : for comment #17 #Messages
Total messages: 25 (12 generated)
Description was changed from ========== wip: Remove observer in destructor BUG= ========== to ========== Remove itself from the observers in MessageCenterBubble::dtor BUG=612050 TEST=msan passes ==========
yoshiki@chromium.org changed reviewers: + oshima@chromium.org
yoshiki@chromium.org changed reviewers: - oshima@chromium.org
Patchset #1 (id:1) has been deleted
Description was changed from ========== Remove itself from the observers in MessageCenterBubble::dtor BUG=612050 TEST=msan passes ========== to ========== Remove itself from the widget observers in destructor This patch may fix the use-after-free reported in crbug.com/612050. BUG=612050 TEST=msan passes ==========
yoshiki@chromium.org changed reviewers: + oshima@chromium.org
Oshima-san, do you think this patch is good?
oshima@chromium.org changed reviewers: + msw@chromium.org
If this is deleted before widget, yes, but I'm not familiar with the destruction order of bubble view stuff. msw@, WDYT?
https://codereview.chromium.org/2031763003/diff/20001/ui/message_center/views... File ui/message_center/views/message_center_bubble.cc (right): https://codereview.chromium.org/2031763003/diff/20001/ui/message_center/views... ui/message_center/views/message_center_bubble.cc:71: bubble_view()->GetWidget()->RemoveObserver(this); Sorry, but TrayBubbleView is "specialized" (ie. not like typical bubbles), and I don't know how it works offhand. Looking at BubbleViewDestroyed, I figured bubble_view() may return null here, but it seems like that's called from the TrayBubbleView dtor, so perhaps not? You might want to ask skuhne@, he probably knows how tray bubbles work far better than myself.
yoshiki@chromium.org changed reviewers: + skuhne@google.com
skuhne@, could you check if this move is ok or not?
skuhne@chromium.org changed reviewers: + mukai@chromium.org, skuhne@chromium.org, xiyuan@chromium.org
Since Mukai is possibly not looking at this anymore I guess that xiyuan might know this too (he worked recently in this area). https://codereview.chromium.org/2031763003/diff/20001/ui/message_center/views... File ui/message_center/views/message_center_bubble.cc (right): https://codereview.chromium.org/2031763003/diff/20001/ui/message_center/views... ui/message_center/views/message_center_bubble.cc:71: bubble_view()->GetWidget()->RemoveObserver(this); My work on bubbles is at least 2 years ago and I did not work on the MessageCenter (or its bubble). It appears that only the TrayBubbleView calls BubbleViewDestroyed in its dtor. I am not sure about this either after glancing for about 15 minutes at it. I would suggest to either let Mukai or xiyuan review. Mukai did possibly write this, but he left Chrome, so I add both to the review.
https://codereview.chromium.org/2031763003/diff/20001/ui/message_center/views... File ui/message_center/views/message_center_bubble.cc (right): https://codereview.chromium.org/2031763003/diff/20001/ui/message_center/views... ui/message_center/views/message_center_bubble.cc:71: bubble_view()->GetWidget()->RemoveObserver(this); It probably would crash since MessageCenterBubble has a different lifetime from the bubble_view()'s widget, i.e. widget could be gone when MessageCenterBubble is released. What does the bug say? I don't have the access to the bug. My guess that that OnWidgetClosing handle the case that widget closes before message bubble but not the other way around, i.e. MessageCenterBubble goes before the bubble Widget. Not sure how it would happen though (maybe signs out with the message center bubble open)?
On 2016/06/20 16:04:40, xiyuan wrote: > https://codereview.chromium.org/2031763003/diff/20001/ui/message_center/views... > File ui/message_center/views/message_center_bubble.cc (right): > > https://codereview.chromium.org/2031763003/diff/20001/ui/message_center/views... > ui/message_center/views/message_center_bubble.cc:71: > bubble_view()->GetWidget()->RemoveObserver(this); > It probably would crash since MessageCenterBubble has a different lifetime from > the bubble_view()'s widget, i.e. widget could be gone when MessageCenterBubble > is released. If I add the null-check for the bubble view and the widget before removing observer, is it ok? eg. if (bubble_view() && bubble_view()->GetWidget()) { bubble_view()->GetWidget()->RemoveObserver(this); } > What does the bug say? I don't have the access to the bug. My guess that that > OnWidgetClosing handle the case that widget closes before message bubble but not > the other way around, i.e. MessageCenterBubble goes before the bubble Widget. > Not sure how it would happen though (maybe signs out with the message center > bubble open)? I added you as CC to the issue, so now you can see it. As described in the bug, I'm not sure how this crash happens either, but the crash is reported actually. Do you think this patch fix the crash described in the issue?
On 2016/06/20 18:04:55, yoshiki wrote:
> If I add the null-check for the bubble view and the widget before removing
> observer, is it ok?
>
> eg.
> if (bubble_view() && bubble_view()->GetWidget()) {
> bubble_view()->GetWidget()->RemoveObserver(this);
> }
>
> > What does the bug say? I don't have the access to the bug. My guess that
that
> > OnWidgetClosing handle the case that widget closes before message bubble but
> not
> > the other way around, i.e. MessageCenterBubble goes before the bubble
Widget.
> > Not sure how it would happen though (maybe signs out with the message center
> > bubble open)?
>
> I added you as CC to the issue, so now you can see it. As described in the
bug,
> I'm not sure how this crash happens either, but the crash is reported
actually.
> Do you think this patch fix the crash described in the issue?
Thanks for sharing the bug. It seems to me that the UAF happens when the
MessageCenterBubble is destoryed without calling its Widget's Close/CloseNow.
Probably happens when you signs out with the MessageCenterBubble open.
Doing it in dtor with null check sounds good. But I think we should keep the one
in OnWidgetClosing or move it to a OnWidgetDestroying so that we don't depends
on the impl details that WebNotificationBubbleWrapper releases its |bubble_|
first before |bubble_wrapper_|.
On 2016/06/20 20:37:08, xiyuan wrote:
> On 2016/06/20 18:04:55, yoshiki wrote:
> > If I add the null-check for the bubble view and the widget before removing
> > observer, is it ok?
> >
> > eg.
> > if (bubble_view() && bubble_view()->GetWidget()) {
> > bubble_view()->GetWidget()->RemoveObserver(this);
> > }
> >
> > > What does the bug say? I don't have the access to the bug. My guess that
> that
> > > OnWidgetClosing handle the case that widget closes before message bubble
but
> > not
> > > the other way around, i.e. MessageCenterBubble goes before the bubble
> Widget.
> > > Not sure how it would happen though (maybe signs out with the message
center
> > > bubble open)?
> >
> > I added you as CC to the issue, so now you can see it. As described in the
> bug,
> > I'm not sure how this crash happens either, but the crash is reported
> actually.
> > Do you think this patch fix the crash described in the issue?
>
> Thanks for sharing the bug. It seems to me that the UAF happens when the
> MessageCenterBubble is destoryed without calling its Widget's Close/CloseNow.
> Probably happens when you signs out with the MessageCenterBubble open.
>
> Doing it in dtor with null check sounds good. But I think we should keep the
one
> in OnWidgetClosing or move it to a OnWidgetDestroying so that we don't depends
> on the impl details that WebNotificationBubbleWrapper releases its |bubble_|
> first before |bubble_wrapper_|.
Thank you for guidance. I put it in the dtor and the OnWidgetClosing with
null-check. PTAL.
lgtm
The CQ bit was checked by yoshiki@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Remove itself from the widget observers in destructor This patch may fix the use-after-free reported in crbug.com/612050. BUG=612050 TEST=msan passes ========== to ========== Remove itself from the widget observers in destructor This patch may fix the use-after-free reported in crbug.com/612050. BUG=612050 TEST=msan passes ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Remove itself from the widget observers in destructor This patch may fix the use-after-free reported in crbug.com/612050. BUG=612050 TEST=msan passes ========== to ========== Remove itself from the widget observers in destructor This patch may fix the use-after-free reported in crbug.com/612050. BUG=612050 TEST=msan passes Committed: https://crrev.com/58f821b32a3b99bf15855225dff13d77f10d7484 Cr-Commit-Position: refs/heads/master@{#402408} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/58f821b32a3b99bf15855225dff13d77f10d7484 Cr-Commit-Position: refs/heads/master@{#402408} |
