Don't repeat the mRefCnt member of URLValue in ImageValue

RESOLVED FIXED in Firefox 40

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla40
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Even after this patch, it's not OK to AddRef an ImageValue and then call
Release on its base pointer (URLValue) since URLValue refcounting methods
are not virtual, so it would confuse the leak checker, but at least it
wouldn't cause UAF issues since we'd still be looking at the same mRefCnt
member.
(Assignee)

Updated

3 years ago
Assignee: nobody → ehsan
Blocks: 1155415
(Assignee)

Comment 1

3 years ago
Created attachment 8597641 [details] [diff] [review]
Don't repeat the mRefCnt member of URLValue in ImageValue
Attachment #8597641 - Flags: review?(roc)
Comment on attachment 8597641 [details] [diff] [review]
Don't repeat the mRefCnt member of URLValue in ImageValue

>-  // Override AddRef and Release to not only log ourselves correctly, but
>-  // also so that we delete correctly without a virtual destructor
>-  NS_INLINE_DECL_REFCOUNTING(ImageValue)
>+  // Override AddRef and Release to log ourselves correctly.
>+  NS_METHOD_(MozExternalRefCountType) AddRef();
>+  NS_METHOD_(MozExternalRefCountType) Release();

Seems like you should leave the comment as it was, no?

r=dbaron with that
Attachment #8597641 - Flags: review?(roc) → review+
Component: Layout → CSS Parsing and Computation
(Assignee)

Comment 3

3 years ago
No, that part of the comment is bogus.  Since the Release method is not virtual in the base class, if you end up destroying an ImageValue through URLValue::Release, you'd end up calling the wrong destructor anyway.  These overrides don't do anything to prevent against that.
But the comment is saying that the reason the override is needed is that if the override were *not* present, there would be no way to release the ImageValue that would call ~ImageValue.
Flags: needinfo?(ehsan)
(Assignee)

Comment 6

3 years ago
(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #4)
> But the comment is saying that the reason the override is needed is that if
> the override were *not* present, there would be no way to release the
> ImageValue that would call ~ImageValue.

Yes, but it also suggests that with the current situation, we would always free ImageValue with calling ~ImageValue, which is not true.  Do you have a better suggestion for the wording of the comment?  I would be happy to fix it.

(FWIW I think the bigger problem with ImageValue is that its refcounting is completely broken because of the above issue, but I can't think of an easy way to fix it, and I have no idea how real the assumption that there's no URLValue* in the program which points to an ImageValue object is.  But fixing this is well beyond the scope of this bug, I think.)
Flags: needinfo?(ehsan) → needinfo?(dbaron)
"delete correctly" -> "delete correctly (assuming callers always call *our* Release method and not our base class's)"


The simple fix for what you're worried about would be to have a URLValueBase that has all of URLValue except for the reference counting bits.
Flags: needinfo?(dbaron) → needinfo?(ehsan)
Blocks: 984780
https://hg.mozilla.org/mozilla-central/rev/c112d4cd63e0
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
(Assignee)

Comment 10

3 years ago
(In reply to Pulsebot from comment #9)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/c5bada9c3efe

Improved the comment: ^
Flags: needinfo?(ehsan)
Blocks: 1272552
You need to log in before you can comment on or make changes to this bug.