1758478 - Incorrect Check before in DataBuffer move assignment

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Danh]

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;
}

Comment 1

[Danh]

Attachment: 1758478.diff

Comment 2

[Dennis Jackson]

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.

Comment 3

[Martin Thomson [:mt:]]

Attachment: Bug 1758478 - Correct check in move assignment, r=djackson

Comment 4

[Danh]

(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

Comment 5

[Danh]

Attachment: 1758478-new.diff

Comment 6

[Danh]

Attachment: cpputil: Correct DataBuffer move constructor

Comment 7

[Danh]

[: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

Comment 8

[Danh]

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

Comment 9

[Martin Thomson [:mt:]]

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.)

Comment 10

[Danh]

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

Comment 11

[Dennis Jackson]

Attachment: Bug 1758478 - Fix DataBuffer Move Assignment. r=mt,sgn

Comment 12

[Dennis Jackson]

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.

Comment 13

[Danh]

(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.

Comment 14

[Dennis Jackson]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Commit for when this landed for 3.77:
https://hg.mozilla.org/projects/nss/rev/f12fd43d69c7b5c7e7dbbaaa2501d370980c2fe5