PtrVector should disallow copies to prevent premature&double deletions

RESOLVED FIXED in Firefox 41

Status

()

Core
WebRTC: Signaling
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gerald, Assigned: gerald)

Tracking

unspecified
mozilla41
x86_64
Windows
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8603974 [details] [diff] [review]
1163505-ptrvector-prevent-copy.patch

Deleted copy constructor&assignment, implemented move constructor&assignment.
Attachment #8603974 - Flags: review?(docfaraday)
(Assignee)

Comment 2

3 years ago
Created attachment 8603975 [details] [diff] [review]
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.
Attachment #8603974 - Attachment is obsolete: true
Attachment #8603974 - Flags: review?(docfaraday)
Attachment #8603975 - Flags: review?(docfaraday)

Comment 3

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

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

This looks fine.
Attachment #8603975 - Flags: review?(docfaraday) → review+
(Assignee)

Comment 4

3 years ago
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()?
Hey gerald - does bug 1161589 look like a dupe of this?
Flags: needinfo?(gsquelart)
(Assignee)

Comment 6

3 years ago
(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...
Flags: needinfo?(gsquelart)
Please make sure you have hg configured to include your author information in new patches as well.
(Assignee)

Comment 12

3 years ago
Created attachment 8606779 [details] [diff] [review]
1163505-ptrvector-prevent-copy.patch

Patch with correct mozilla::Move instead of std::move.
Attachment #8603975 - Attachment is obsolete: true
Attachment #8606779 - Flags: review+
(Assignee)

Comment 13

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=13b86813992c
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2d331d4ba0f5
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.