clang-analyzer-cplusplus.NewDelete false-positive - does not note nullification of ptr after delete

[Quuxplusone]

Bugzilla Link	PR44050
Status	NEW
Importance	P enhancement
Reported by	Roman Lebedev (lebedev.ri@gmail.com)
Reported on	2019-11-18 12:42:01 -0800
Last modified on	2019-11-20 14:39:04 -0800
Version	trunk
Hardware	PC Linux
CC	dcoughlin@apple.com, llvm-bugs@lists.llvm.org, martongabesz@gmail.com, noqnoqneo@gmail.com
Fixed by commit(s)
Attachments	RawImage.cpp.xz (126756 bytes, application/x-xz)
Blocks
Blocked by
See also

This is manually reduced from a report that i got via CTU mode (yay).
The issue appears to be false-positive:

struct S {
    int *storage;
    ~S() {
        if(!storage)
            return;
        delete storage; // <- we can't
        storage = nullptr;
    }
};

S producer();

S foo() {
    S imm = producer();
    return imm;
}

S bar() {
    S imm = foo();
    return imm;
}

warning: Attempt to free released memory [clang-analyzer-cplusplus.NewDelete]
        delete storage;
        ^

But how can that happen, after deleting we set it to nullptr?

https://godbolt.org/z/Jkusgy

[Quuxplusone]

The warning on reduced code looks correct to me. The write to 'storage' in the destructor is a dead store. Nobody will ever read from this memory because the lifetime of the object ends with the destructor. The old value of the 'storage' is already copied over to the new object by the time it's reassigned to nullptr. So the destructor of the new object will delete it again.

The only problem i see with the reduced test case is that the static analyzer shouldn't really see any copies in this code: due to RVO and NRVO, no copies will really occur in this code in practice, and the return value of 'producer()' will be constructed directly at the return site of 'bar()'. We currently support RVO but not NRVO (http://clang-analyzer.llvm.org/open_projects.html => "Handle constructors that can be elided due to Named Return Value Optimization (NRVO)"), so we perform multiple copy constructors under the hood while modeling this code. But NRVO is optional (even in C++17), so i'm not super worried.

I think there was something interesting in the copy/move constructor in the original code that made the issue impossible.

[Quuxplusone]

(In reply to Artem Dergachev from comment #1)
> I think there was something interesting in the copy/move constructor in the
> original code that made the issue impossible.

Yes, i suppose so.
FWIW the original destructor is:
https://github.com/darktable-org/rawspeed/blob/9365ac01db344d6f4689f68e6b51e14e835a4a29/src/librawspeed/common/RawImage.cpp#L304-L316
(There's likely an race condition, yes)

[Quuxplusone]

Aha, we have a fundamental problem with reference counting pointers that consists in occasionally discovering execution paths on which the reference count overflows or underflows or was ill-formed from the beginning, which doesn't usually happen in practice. It's probably one of these. We have a couple of crude suppressions for these false positives. I could take a look at why didn't they kick in if i manage to take a look at a preprocessed file. Given that i already have source code for the implementation of the smart pointer, a simple code that uses these pointers to demonstrate the problem could do fine.

[Quuxplusone]

Attached RawImage.cpp.xz (126756 bytes, application/x-xz): preprocessed source

[Quuxplusone]

Hmm, doesn't warn for me. I guess, one does not simply reproduce a CTU bug with a preprocessed file :(

In any case, i think i see what the problem is, i can try to fix this blindly.

[Quuxplusone]

(In reply to Artem Dergachev from comment #5)
> Hmm, doesn't warn for me. I guess, one does not simply reproduce a CTU bug
> with a preprocessed file :(
Right, this warning did not show up without CTU, it requires some other TU to
manifest.

> In any case, i think i see what the problem is, i can try to fix this
> blindly.
Thank you, i should be able to assess the effect of the patch.