1163505 - PtrVector should disallow copies to prevent premature&double deletions

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

PtrVector holds a vector of pointers, and deletes all pointers on destruction.
It also implicitly allows copies, which could result in two PtrVector's both pointing to the same objects, causing a double deletion; Or if one copy is deleted, users of the second copy will now see deleted objects.

This does happens in a debug version of JsepSessionImpl::CreateAnswerMSection(), which calls JsepSessionImpl::GetCommonCodecs() to retrieve a PtrVector containing pointers to clones of codecs. However as the 'return' from GetCommonCodecs does not use tail-return-optimization in debug mode, it creates a copy of the PtrVector and destroys one; the program crashes a bit later when trying to use a deleted codec object.

Copies should explicitly be disabled, and moves implemented to transfer ownership of the pointers as appropriate.

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1163505-ptrvector-prevent-copy.patch

Deleted copy constructor&assignment, implemented move constructor&assignment.

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1163505-ptrvector-prevent-copy.patch

Apologies, previous patch had wrong wrong wrong implementation of assignment! (But it's not used, that's why the compiler didn't complain)

Deleted copy constructor&assignment, implemented move constructor&assignment.

Comment 3

[Byron Campen [:bwc]]

Comment on attachment 8603975 [details] [diff] [review]
1163505-ptrvector-prevent-copy.patch

Review of attachment 8603975 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine.

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Found this note in https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code:
"""rvalue references: Implicit move method generation cannot be used."""

Could this be why PtrVector was not implicitly moved when returned from JsepSessionImpl::GetCommonCodecs()?

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Hey gerald - does bug 1161589 look like a dupe of this?

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #5)
> Hey gerald - does bug 1161589 look like a dupe of this?

No. However, 'my' other recent bug 1163456 is a definite dupe of bug 1161589! Will link them...

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=13b86813992c

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Please make sure you have hg configured to include your author information in new patches as well.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/41f03ff16f47

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://treeherder.mozilla.org/logviewer.html#?job_id=9918426&repo=mozilla-inbound

Comment 11

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b92f853d8cc

Comment 12

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1163505-ptrvector-prevent-copy.patch

Patch with correct mozilla::Move instead of std::move.

Comment 13

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Sorry for the snafu, forgot to upload the correct patch after successful try.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=13b86813992c

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d331d4ba0f5

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2d331d4ba0f5