Hi all
we are analyzing (you see, with ‘z’) a few hundred thousands lines of desktop c++ code on sonarcloud and we have 173 instances of S3584, out of 100 that I looked at so far, only 2 are possibly not false-positives.
This noise is due to the fact that the QT framework will automatically destroy instances of QObjects whose parents have been destroyed. This is probably the defining feature of the framework (along with signals) and QT is THE gui framework for C++ development.
In other words, the following pattern (where MainWindow is a QObject and QProgressDialog is also a QObject) is used all the time and it’s a false-positive all the time:
void MainWindow::showProgressDialog()
{
QProgressDialog* progress = new QProgressDialog(this);
// do other stuff
progress->show();
}
I know that you don’t often support framework features, but QT is a huge thing for desktop development and this noise is truly painful. Navigating the class hierarchy to check if it’s rooted in QObject would probably kill all these false-positives, while doing the same for the first parameter passed in the constructor would restore most of the possible trues. If class hierarchy navigation is a problem, at least providing an exclusion pattern (since often there’s a naming convention associated to the usage of QObject-derived classes) would allow reducing the false-positives in a simpler way.
I’m not sure if the cost of implementing special cases for a framework is worth it, but before going into that discussion, let’s refine what would be needed a little bit.
The goal is to remove false positives, but not to sacrifice real cases. Therefore, expanding on your example:
void MainWindow::showProgressDialog()
{
QProgressDialog* progress1 = new QProgressDialog(this); // Ok
QProgressDialog* progress2 = new QProgressDialog(); // Leak
QProgressDialog* progress3 = new QProgressDialog("Progress", "Cancel", 0, 100, this); // Ok
QProgressDialog* progress4 = new QProgressDialog("Progress", "Cancel", 0, 100); // Leak
}
I agree that blindly excluding all classes that inherit from QObject would prevent detecting very interesting cases.
So the analyzer should detect cases where an object (of a type that inherits from QObject) is created by new, and the object constructor has a non null argument for its “parent” parameter.
And the next question is how to detect the parent parameter. It’s not always the first parameter. But it’s a parameter of a type that inherits from QObject. Is that enough (I’ve not used Qt for some time, so I’m not sure…), or would we need another disambiguation (for instance, the parameter is named parent, which seems a little bit brittle…)?
Hello Loic,
thanks for the answer.
If we assume (for simplicity’s sake) that the rule is finding all memory leaks in our code, sensitivity is 1, great; but precision is 0.02 (2/100). At such low precision, sacrificing true positives is a small price to pay. This, of course, is only true for a tool that is supposed to be used everyday by developers; the matter would be different if sonar was exclusively a tool to be used by a security expert once during an assessment, where sensitivity beats accuracy all the time.
Checking for the root class of the created object to be QObject and that there is at least one instance of a QObject being passed as parameter into the constructor, for our dataset would keep sensitivity to 1 and bring precision to 1. You would also get sensitivity 1 and precision 1 in the 4 examples you listed.
This is what the code would have to look like for the logic above to miss a positive and generate a false negative :
void MainWindow::showProgressDialog()
{
QProgressDialog* leakingProgress = new QProgressDialog(this, NULL); // Unfortunately parent was the second parameter!
}
It’s a pity for this corner-case, for sure, but it’s much more a pity that, for the average QT user, this rule is 90%+ noise.
Wdyt?
We already have an open ticket on this issue, I have updated it with information coming from this discussion.
That being said, tuning a rule for Qt is not currently at the top of our priorities, so I cannot make any forecast on when this ticket is going to be worked on.
If other people are facing this issue, please let us know so that we may update our priorities.
