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.
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+
4 years ago
Component: Layout → CSS Parsing and Computation
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.
4 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)
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
(In reply to Pulsebot from comment #9) > https://hg.mozilla.org/integration/mozilla-inbound/rev/c5bada9c3efe Improved the comment: ^
You need to log in before you can comment on or make changes to this bug.