We have a large C++ application and use SonarQube to improve code quality and catch bugs. Recently we noticed a memory leak: in one location we want to free the value of a unique_ptr and set it to the null pointer, however by accident we used unique_ptr::release() instead of the intended unique_ptr::reset().
In valid uses of unique_ptr::release() the programmer will use the return value and manually free it. A call to release without using the return value is certainly a mistake.
We were surprised that SonarQube did not catch this mistake and also did not find an optional C++ rule we could enable.
I’d like to propose to add a SonarQube rule that warns if release() is called on a unique_ptr but the return value is ignored.
#include <iostream>
#include <memory>
int main()
{
std::unique_ptr<std::string> person = std::make_unique<std::string>("Grace Hopper");
// Release ownership of the pointer, replacing it with the null pointer,
// but ignore the return value. This causes a memory leak.
person.release();
// Either use unique_ptr::reset() to set person to the null pointer
// and also free the value, or assign the returned value to a variable:
// std::string* name = person.release();
// std::cout << *name << std::endl;
return 0;
}
I’m curious to hear more about your leak. The example you shared is likely a reduction of the actual case of yours, but probably has lost some context because as you can see Compiler Explorer, we would find the leak in your short example.
You can create a reproducer by following these instructions, and let me know if you share it privately - in which case I’d contact you.
To generate the reproducer file:
Search in the analysis log for the full path of the source file for which you want to create a reproducer (for instance, a file that contains a false-positive). You will have to use exactly this name (same case, / or \…)
Add the reproducer option to the scanner configuration:
sonar.cfamily.reproducer=“Full path to the .source file”
Re-run the scanner to generate a file named sonar-cfamily-reproducer.zip in the project folder.
Please share this file. If you think this file contains private information, let us know, and we’ll send you a private message that will allow you to send it privately.
Thank you! You correctly assume that I shared a reduced example. Unfortunately I am not allowed to share project code. I didn’t know I could use Sonar in the compiler explorer. That is very useful.
SonarQube appears to do symbolic execution in order to detect the memory leak. In an example with only a call to release() the warning isn’t triggered: Compiler Explorer. I guess that our code is too complicated so the symbolic execution engine gives up before the potential memory leak is detected. Our SonarQube server is still on version 10.3, so we should really update and test if it detects the memory leak in the current release.
[…] In an example with only a call to release() the warning isn’t triggered: Compiler Explorer.
Thank you for sharing this interesting case. We intentionally don’t raise the issue in that case if we don’t see the allocation.
So, if the constructor is not defined in the given translation unit, or the other function calls are not visible initializing the unique_ptr field, then we conservatively assume that no allocation happened.
The more interesting question to me would be, if we don’t see an allocation, but we see a dereference, then why we don’t assume it’s allocated? For example:
#include <memory>
void top(std::unique_ptr<int> p) {
*p = 404; // Clearly, this would be invalid if p was null.
p.release(); // Hmm, so, p is non-null and we release it? It's a LEAK!
}
I’ve created the CPP-6168 for tracking this potential improvement for catching this false-negative (FN) case.
I think that, in addition to the advanced detection of missing deallocation mentioned by @balazs.benics, there is also room for a more simple rule that would detect unused return value of unique_ptr::release, as if this function was marked as [[nodiscard]].
This is happening in our case. One project (compilation unit) defines the class but it is only used from another project. So while the unique_ptr is both allocated and released in the compilation unit static analysis correctly observes that these functions aren’t called together in current project.
This simple rule would have found the bug and would be a very welcome addition to SonarQube.