1158540 - Don't repeat the mRefCnt member of URLValue in ImageValue

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[(no longer active)]

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.

Comment 1

[(no longer active)]

Attachment: Don't repeat the mRefCnt member of URLValue in ImageValue

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 3

[(no longer active)]

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.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c112d4cd63e0

Comment 6

[(no longer active)]

(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.)

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

"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.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c112d4cd63e0

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5bada9c3efe

Comment 10

[(no longer active)]

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

Improved the comment: ^

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c5bada9c3efe