1278201 - [Static Analysis][Write to pointer after free] In function Release()

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Andi [:andi]]

The Static Analysis tool Coverity added that |mRefCnt| is written after is freed:

>>    if (mRefCnt == 1) {
>>      delete this;
>>    }
>>    mRefCnt--;

I think the correct approach is:

>>    mRefCnt--;
>>    if (mRefCnt == 0) {
>>      delete this;
>>    }
>>    return mRefCnt;

Comment 1

[Andi [:andi]]

Attachment: Bug 1278201 - prevent writing |mRefCnt| after it's freed.

Review commit: https://reviewboard.mozilla.org/r/57870/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57870/

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://reviewboard.mozilla.org/r/57870/#review54752

::: mfbt/tests/TestNotNull.cpp:74
(Diff revision 1)
> -    if (mRefCnt == 1) {
> +    mRefCnt--;
> +    if (mRefCnt == 0) {
>        delete this;
>      }
> -    mRefCnt--;
>      return mRefCnt;

This still sounds like a bad idea.

Comment 3

[Andi [:andi]]

Comment on attachment 8760183 [details]
Bug 1278201 - prevent writing |mRefCnt| after it's freed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57870/diff/1-2/

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8760183 [details]
Bug 1278201 - prevent writing |mRefCnt| after it's freed.

https://reviewboard.mozilla.org/r/57870/#review54788

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0773860ce60c
prevent writing |mRefCnt| after it's freed. r=Ms2ger

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0773860ce60c