1574971 - Consider using weak ptrs for mSegmentReader/mSegmentWriter in h2 code

Status: RESOLVED FIXED

(Core :: Networking: HTTP, task, P2)

Description

[Honza Bambas (:mayhemer)]

Currently those are raw ptrs. We never nullify them and we use them expecting to be non-null. When the outer class is released, those members keep refs to released memory, causing e.g. UAF bug 1402014.

I think these are single thread used (socket thread) so weak ptr would do well here to avoid cycles and UAFs.

Comment 1

[Honza Bambas (:mayhemer)]

Existing implementers of nsAHttpSegmentReader/nsAHttpSegmentWriter already mix* nsSupportsWeakReference and SupportsWeakPtr<T>. We need to consolidate first, I vote for SupportsWeakPtr<> . It's more modern.

• • nsHttpConnection was added along with HalfOpen nsSupportsWeakReference in bug bug 1341572.

Comment 2

[Honza Bambas (:mayhemer)]

Making base classes SupportsWeakPtr-derived + the final class again SupportsWeakPtr-derived (for it's own type) causes a compilation error for SelfReferencingWeakPtr() being declared on base and derived again with only the return type differing.

I tried some template EnableIf magic, but that doesn't work as I'd like to.

Comment 3

[Honza Bambas (:mayhemer)]

Attachment: Bug 1574971, draft

Comment 4

[Honza Bambas (:mayhemer)]

Nathan, FYI, this bug is the main motivation for bug 1580182. I don't want to add dependencies to open bugs here.

Comment 5

[Honza Bambas (:mayhemer)]

Attachment: Bug 1574971 - Migrate some network classes to use multiple inheritance from SupportsWeakPtr<>

Comment 6

[Honza Bambas (:mayhemer)]

Attachment: Bug 1574971 - Turn segment reader and writer refrences to WeakPtrs where appropriate

Depends on D45882

Comment 7

[Honza Bambas (:mayhemer)]

(add more info what blocks me)

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #7)

(add more info what blocks me)

I'll go with Comment 6 and see how it goes.

Comment 9

[Honza Bambas (:mayhemer)]

Releasing this bug from now, I don't think we have an immediate problem here. This could be actually considered an enhancement/improvement of the existing code only.

Comment 10

[Valentin Gosu [:valentin] (he/him)]

Fixed in bug 1667102 (I think).