C++: Reduce number of FPs cpp/guarded-free and turn if(x) { free(x) } cases from FNs to TPs

[jketema]

The additional conditions to reduce FPs are maybe somewhat on the strict side, but given that a MRVA 1000 experiment already gives over 5k results, I don't think that this much of a problem. As far as I can tell there is just one remaining FP in MRVA (which occurs a few times), and which comes from the libpng library.

https://github.com/naturalatlas/node-gdal/blob/c83e7858a9ec566cc91d65db74fd07b99789c0f0/deps/libgdal/gdal/frmts/png/libpng/pngmem.c#L541-L560

Given that this is now the only FP, I did not investigate this further. I think we could promote the query with these changes from this PR.

Pull Request checklist

All query authors

• A change note is added if necessary. See the documentation in this repository.
• All new queries have appropriate .qhelp. See the documentation in this repository.
• QL tests are added if necessary. See Testing custom queries in the GitHub documentation.
• New and changed queries have correct query metadata. See the documentation in this repository.

Internal query authors only

• Autofixes generated based on these changes are valid, only needed if this PR makes significant changes to .ql, .qll, or .qhelp files. See the documentation (internal access required).
• Changes are validated at scale (internal access required).
• Adding a new query? Consider also adding the query to autofix.

[geoffw0]

Couple of small requests, otherwise LGTM.

Also deserves a DCA run before merging. I gather you have reviewed MRVA differences already.

[geoffw0]

I don't particularly like this exclusion, but I can see that it circumvents some really difficult cases and helps get us a good set of results.

[jketema]

Agreed.

[jketema]

Also deserves a DCA run before merging.

How do you propose I run DCA? Note that this is still an experimental query, so it won't be run by default.

I gather you have reviewed MRVA differences already.

Correct. Checking for the file when in the proprocessor block case, might deserve some further investigation though.

[geoffw0]

How do you propose I run DCA? Note that this is still an experimental query, so it won't be run by default.

Ah, perhaps we can save this until the PR that promotes the query then. FWIW I think it is possible to specify a single query rather than a suite when running DCA - if so, that might work around the problem, though I guess nothing would be cached so the performance data might still be a bit off.

Checking for the file when in the proprocessor block case, might deserve some further investigation though.

Yeah, this bug could have been almost randomly hiding results - good or bad.

[geoffw0]

👍

Approved, on the assumption that you briefly re-check some results on MRVA following the fix to blockContainsPreprocessorBranches.

[jketema]

FWIW I think it is possible to specify a single query rather than a suite when running DCA

Good point. Let me run that anyway, so we can at least see if there are any alert differences, and it doesn't blow up completely analysis time-wise.

[geoffw0]

DCA shows 99 added, 2866 removed results for cpp/guarded-free. Unfortunately we don't seem to be able to see the individual changes, but I suppose we were expecting some new results for the { free(x); } pattern. Analysis time looks to be unaffected. 👍

[jketema]

Approved, on the assumption that you briefly re-check some results on MRVA following the fix to blockContainsPreprocessorBranches.

There are about 1.6k new results. They all look genuine.

DCA shows 99 added, 2866 removed results for cpp/guarded-free. Unfortunately we don't seem to be able to see the individual changes, but I suppose we were expecting some new results for the { free(x); } pattern. Analysis time looks to be unaffected. 👍

Yup. I've asked internally whether I can still somehow generate the table.

[jketema]

I spot checked some of the alerts that disappeared in DCA. Most of them seem to be due to

 strictcount(BasicBlock bb2 | gc.ensuresEq(_, 0, bb2, _) | bb2) = 1

And it's correct that they disappeared, because the code depends on the guard being there.