Closed Bug 1758478 Opened 2 years ago Closed 2 years ago

Incorrect Check before in DataBuffer move assignment

Categories

(NSS :: Libraries, defect, P3)

3.76

Tracking

(firefox-esr91 unaffected, firefox98 wontfix, firefox99 wontfix, firefox100 fixed)

RESOLVED FIXED
Tracking Status
firefox-esr91 --- unaffected
firefox98 --- wontfix
firefox99 --- wontfix
firefox100 --- fixed

People

(Reporter: congdanhqx, Assigned: djackson)

References

(Regression)

Details

(Keywords: regression)

Attachments

(4 files, 1 obsolete file)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:97.0) Gecko/20100101 Firefox/97.0

Steps to reproduce:

Review Code

Actual results:

The Move Assignment only move from self, then zero-ing self:
https://github.com/nss-dev/nss/blob/master/cpputil/databuffer.h#L36

DataBuffer& operator=(DataBuffer&& other) {
if (this == &other) {
data_ = other.data_;
len_ = other.len_;
other.data_ = nullptr;
other.len_ = 0;
}
return *this;
}

Expected results:

DataBuffer& operator=(DataBuffer&& other) {
if (this != &other) {
data_ = other.data_;
len_ = other.len_;
other.data_ = nullptr;
other.len_ = 0;
}
return *this;
}

Attached patch 1758478.diffSplinter Review

Thanks Danh, this does indeed look like a typo to me, but I know little of C++ move constructors.

If I understand correctly, I think we also need to clear the memory held in this.data_, otherwise it will leak.

Severity: -- → S4
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(mt)
Priority: -- → P3
Regressed by: 1747310

(In reply to Dennis Jackson from comment #2)

If I understand correctly, I think we also need to clear the memory held in this.data_, otherwise it will leak.

Absolutely, I attached new revision with this comment

Attached patch 1758478-new.diffSplinter Review

[:mt:] The diff in https://phabricator.services.mozilla.com/D140573 is problematic. If copy-ctor is called, data_ will be double-free, one in "delete[] data_" in copy-ctor, one in Assign. I think it's better to use https://phabricator.services.mozilla.com/D140605

I can't comment in D140573

Has Regression Range: --- → yes

Hello @mt, I still see a "delete[] data_" in assignment operator, I think it's incorrect, too, same arguments as before (double free).

Hi Danh, I removed the delete from the move assignment (operator=(T&&)) but not the copy assignment (operator=(const T&)). I think that's right as the move does a swap, but the copy doesn't destroy the value it is copying from. (The old code was safe only because we never assigned twice, but that's no longer a safe assumption.)

Flags: needinfo?(mt)

I meant the data_ will be deleted twice, one is the newly added, one in Allocate (via Assign)

Running D140573 against the TLS gtests does indeed lead to a double-free as Dan suggested. Running D140605 with valgrind doesn't find any leaked memory.

I'm not sure what the motivation is for std::swap, I'd rather leave the old object in a cleared state. As-is in both patches the old object points to the data we thought we were overwriting which feels like a recipe for hard to track down errors.

Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED

(In reply to Dennis Jackson from comment #12)

Running D140573 against the TLS gtests does indeed lead to a double-free as Dan suggested. Running D140605 with valgrind doesn't find any leaked memory.

I'm not sure what the motivation is for std::swap, I'd rather leave the old object in a cleared state. As-is in both patches the old object points to the data we thought we were overwriting which feels like a recipe for hard to track down errors.

The motivation was keeping "DataBuffer::data_" always valid as long as DataBuffer valid. Since the swap would be very fast, the free-ing of data_ will be postponed (until the destruction of temporary "DataBuffer&& other").

I'm not sure if DataBuffer will be used in any multi-thread manner, if not, then nothing to worry about.

Ah thanks for clarifying and helping out with this Dan! DataBuffer isn't a thread-safe class.

Assignee: nobody → djackson
Target Milestone: --- → 3.77
Attachment #9266917 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.