C++: Improve and promote cpp/overflow-buffer

[geoffw0]

Improve and promote "Call to memory access function may overflow buffer" (cpp/overflow-buffer).

• fix an issue where references to array expressions inside offsetof expressions were being misinterpreted as accesses.
• promote the query onto security-extended (by increasing the severity to warning and setting the precision to medium).
• added some test cases along the way, inspired by real world results / FPs found using MRVA.
  • I actually found three classes of false positives in the real world results, but only decided to fix one of them. For the other two, I believe it will be more challenging to determine which are TP vs FP cases. If we want to promote the query to high precision (code scanning suite) at some point, we will probably need to do something with these.

PR Overview

This PR improves and promotes the "Call to memory access function may overflow buffer" query by fixing its handling of array expressions within offsetof and adjusting its severity and precision for the security-extended suite.

• Updated change note in cpp/ql/src documenting the security-extended promotion of cpp/overflow-buffer.
• Added a change note in cpp/ql/lib for fixing the getBufferSize predicate issue.

Reviewed Changes

File	Description
cpp/ql/src/change-notes/2025-02-20-overflow-buffer.md	Added change note to promote cpp/overflow-buffer to security-extended.
cpp/ql/lib/change-notes/2025-02-20-getbuffersize.md	Documented the fix for getBufferSize misinterpreting array expressions.

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more

[jketema]

Did you look at the results flagged up by DCA and their quality?

[geoffw0]

DCA

• failed due to WebKit-JavaScriptCore; it shouldn't have been run, but probably my DCA was still the build from https://github.com/github/codeql-dca/pull/2418 so it did run. Irrelevant to this PR.
• 1164 results on dsp-testing__samate-c-cpp
  • sample were all in the bad test cases, i.e. these results look great!
• a light scatter of results on other projects
  • most of which are of the "worth checking what's going on here" rather than "definitely wrong" type; accessing offset pointers using negative indices (update: looking again, at least some of these are TP) and clearing consecutive elements of a struct (update: on closer inspection the latter results look to be doing something unsafe after all) being common patterns. This is probably OK as we're only targetting medium precision and there isn't a huge amount of noise - but I'm going to add a couple more test cases and see if I can narrow down results a little more.

[geoffw0]

I think this is ready to merge, but I'd really appreciate a second opinion on at least some of the real world results.

[jketema]

This is what I'm seeing:

• neovim: likely a FP, but hard to tell because there's quite a lot of pointer manipulation going on
• PHP: These look like FPs. One of the patterns here is memset(p->free_map, 0, sizeof(p->free_map) + sizeof(p->map)) Are we handling sizeof correctly (see also below)?
• OTP: The alert in erl_bif_info.c looks like a TP. The hashing ones are difficult to follow so not sure.
• TinyUSB: These all look like FPs of the form memset(arr, 0, sizeof(arr)). Are we handling sizeof correctly?

[geoffw0]

Thanks for taking a look.

PHP: These look like FPs. One of the patterns here is memset(p->free_map, 0, sizeof(p->free_map) + sizeof(p->map)) Are we handling sizeof correctly (see also below)?

I've just pushed a change that fixes this FP result in PHP (amongst others), and the cost of losing some TPs as well. I think this is a good tradeoff given the query is a bit noisy, and one we could improve in future by refining the existing getRootType predicate in Buffer.qll to cope with a wider range of things.

OTP: The alert in erl_bif_info.c looks like a TP. The hashing ones are difficult to follow so not sure.

The hashing ones are also eliminated by the commit I just pushed (the clear TP result remains).

[geoffw0]

TinyUSB: These all look like FPs of the form memset(arr, 0, sizeof(arr)). Are we handling sizeof correctly?

This was caused by a variable with multiple types in the database. Probably shouldn't happen, but I've hardened the query to this.

I should do another DCA run now with all the changes... [started]

[geoffw0]

One of the tests is from the internal repo so I'm moving it to here before accepting the differences. I think I'll also need to merge main (after the old copy of the test is deleted) before all tests actually pass here...

[jketema]

Thanks for the fixes. I'm happy with this once DCA comes showing that we no longer unexpectedly lose or gain results.

[geoffw0]

Hmm, I'm still seeing some cases with negative sizes in the latest DCA run. :(

[jketema]

Hmm, I'm still seeing some cases with negative sizes in the latest DCA run. :(

Shall we just back out the changes from 1354beb (and the fix you did on top of that)?

[geoffw0]

My Friday brain appears to have come up with an "obvious" better solution, so we'll see how we do with that.

The problem with backing out the changes is having spend time looking into the results I'm not really sure any more that the query should be promoted without both improvements. So that leaves us with the offsetof bug fix and not a lot else. We shall see...

[geoffw0]

DCA

• cpp/static-buffer-overflow on SAMATE: we go from 72 results to 0; this is due to a philosophical difference about overwriting multiple elements of a struct in one operation (we could make a "strict" version of the query if we really want to cover these cases, though I suspect demand for it will be low outside of SAMATE scoring).
• cpp/overflow-buffer on SAMATE: we go from 11,388 results to 1,020; but there are a lot of duplicate results in the former, if we look at unique alert lines we get 1,164 results before to 1,020 results after. I imagine we've lost similar / overlapping results as with cpp/static-buffer-overflow. All results I checked both lost and retained were in "bad" test cases so likely TP.
• we also lost a few results for cpp/overflow-buffer across other projects, which appear to be FPs, and gained a result (on kamailio) that appears to be TP. 🎉

TL;DR: not perfect but should be an improvement for users of the query.

[jketema]

Happy that everything looks ok now.