Fix PLDHashTable::operator=

RESOLVED FIXED in Firefox 40

Status

()

Core
XPCOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla40
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
As per bug 1149888 comment 10, PLDHashTable::operator= has some problems.
(Assignee)

Comment 1

3 years ago
Created attachment 8600232 [details] [diff] [review]
Fix PLDHashTable::operator=

This fixes the following problems with PLDHashTable::operator=:

- It doesn't handle self-assigments.

- It leaks the memory used by the assigned-to table.

- It doesn't leave the assigned-from table in a safely destructable state.

It also adds some test code for operator=.
Attachment #8600232 - Flags: review?(nfroyd)
(Assignee)

Comment 2

3 years ago
Here's Valgrind output for the new test code when the old operator= function is used with it. The first two complaints are due to problem 3 from comment 1, and the leak is due to problem 2.

> ==6116== Invalid read of size 4
> ==6116==    at 0x4151CF: Finish (pldhash.cpp:317)
> ==6116==    by 0x4151CF: PL_DHashTableFinish(PLDHashTable*) (pldhash.cpp:337)
> ==6116==    by 0x40479F: TestPLDHash::test_pldhash_move_constructor() (TestPLDHash.cpp:148)
> ==6116==    by 0x4047DC: main (TestPLDHash.cpp:224)
> ==6116==  Address 0x139feb10 is 0 bytes inside a block of size 128 free'd
> ==6116==    at 0x4C2BE10: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==6116==    by 0x4151FC: Finish (pldhash.cpp:330)
> ==6116==    by 0x4151FC: PL_DHashTableFinish(PLDHashTable*) (pldhash.cpp:337)
> ==6116==    by 0x404797: TestPLDHash::test_pldhash_move_constructor() (TestPLDHash.cpp:147)
>   6116==    by 0x4047DC: main (TestPLDHash.cpp:224)
> ==6116== 
> ==6116== Invalid free() / delete / delete[] / realloc()
> ==6116==    at 0x4C2BE10: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==6116==    by 0x4151FC: Finish (pldhash.cpp:330)
> ==6116==    by 0x4151FC: PL_DHashTableFinish(PLDHashTable*) (pldhash.cpp:337)
> ==6116==    by 0x40479F: TestPLDHash::test_pldhash_move_constructor() (TestPLDHash.cpp:148)
> ==6116==    by 0x4047DC: main (TestPLDHash.cpp:224)
> ==6116==  Address 0x139feb10 is 0 bytes inside a block of size 128 free'd
> ==6116==    at 0x4C2BE10: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==6116==    by 0x4151FC: Finish (pldhash.cpp:330)
> ==6116==    by 0x4151FC: PL_DHashTableFinish(PLDHashTable*) (pldhash.cpp:337)
> ==6116==    by 0x404797: TestPLDHash::test_pldhash_move_constructor() (TestPLDHash.cpp:147)
> ==6116==    by 0x4047DC: main (TestPLDHash.cpp:224)
> ==6116== 
> test_pldhash_assignment_operator : SUCCESS
> ==6116== 
> ==6116== HEAP SUMMARY:
> ==6116==     in use at exit: 59,928 bytes in 448 blocks
> ==6116==   total heap usage: 999 allocs, 552 frees, 97,057 bytes allocated
> ==6116== 
> ==6116== 128 bytes in 1 blocks are definitely lost in loss record 396 of 441
> ==6116==    at 0x4C2ABA0: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==6116==    by 0x415C19: Add (pldhash.cpp:574)
> ==6116==    by 0x415C19: Add (pldhash.cpp:645)
> ==6116==    by 0x415C19: PL_DHashTableAdd(PLDHashTable*, void const*) (pldhash.cpp:710)
> ==6116==    by 0x4046EC: TestPLDHash::test_pldhash_move_constructor() (TestPLDHash.cpp:139)
> ==6116==    by 0x4047DC: main (TestPLDHash.cpp:224)

The new code triggers no Valgrind errors.
(Assignee)

Comment 3

3 years ago
Whoa, try server is unhappy: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fa7564e15f8

I'll investigate on Monday.
Comment on attachment 8600232 [details] [diff] [review]
Fix PLDHashTable::operator=

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

Since try is so red, I'm going to cancel this review; I don't see what the immediate issue is, but...

::: xpcom/glue/pldhash.cpp
@@ +271,5 @@
> +  using mozilla::Move;
> +
> +  if (this != &aOther) {
> +    // Destruct |this|.
> +    Finish();

It looks like something is most unhappy with this Finish() call.
Attachment #8600232 - Flags: review?(nfroyd)
(Assignee)

Comment 5

3 years ago
Created attachment 8600735 [details] [diff] [review]
Fix PLDHashTable::operator=

I added an initializer list to the move constructor, which was necessary so
that the call to the assignment operator wasn't looking at uninitialized
values. And I expanded the test code to cover this case.

I also made Finish() ok to call even if Init() hasn't been called --
this will be needed when I add a destructor, anyway.
Attachment #8600735 - Flags: review?(nfroyd)
(Assignee)

Updated

3 years ago
Attachment #8600232 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Blocks: 1128407
(Assignee)

Updated

3 years ago
Blocks: 1161377
Comment on attachment 8600735 [details] [diff] [review]
Fix PLDHashTable::operator=

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

Thanks for fixing this up.

::: xpcom/glue/pldhash.cpp
@@ +267,5 @@
>  }
>  
> +PLDHashTable& PLDHashTable::operator=(PLDHashTable&& aOther)
> +{
> +  using mozilla::Move;

Nit: this can go away, since we're already |using namespace mozilla;| in this file.

@@ +269,5 @@
> +PLDHashTable& PLDHashTable::operator=(PLDHashTable&& aOther)
> +{
> +  using mozilla::Move;
> +
> +  if (this != &aOther) {

Do we really need to handle self-assignment here?  I know it's Good C++ Practice and all, but there's a ton of code in the tree that doesn't handle this.

Also, I think we should write this as:

if (this == &aOther) {
  return *this;
}

and then proceed; handling exceptional cases first and all that.
Attachment #8600735 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 8

3 years ago
> Do we really need to handle self-assignment here?  I know it's Good C++
> Practice and all, but there's a ton of code in the tree that doesn't handle
> this.

I'll favour correctness over consistency with the existing code! :)

> Also, I think we should write this as:
> 
> if (this == &aOther) {
>   return *this;
> }
> 
> and then proceed; handling exceptional cases first and all that.

Good idea.
https://hg.mozilla.org/mozilla-central/rev/168f09fc229f
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.