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

[LebedevRI]

Bugzilla Link	44050
Version	trunk
OS	Linux
CC	@devincoughlin,@martong,@haoNoQ

Extended Description

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

[LebedevRI]

assigned to @haoNoQ

[haoNoQ]

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.

[LebedevRI]

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)

[haoNoQ]

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.

[LebedevRI]

preprocessed source

[haoNoQ]

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.

[LebedevRI]

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.