C++: Promote cpp/access-memory-location-after-end-buffer-strncat out of experimental
#5938
Conversation
…nd-buffer-strncat' into 'cpp/unsafe-strncat'.
…rncat' into the tests from 'cpp/unsafe-strncat'.
MathiasVP
added
C++
ready-for-doc-review
|
|
📚 Thanks for the docs ping! 🛎️ This was added to our docs first-responder project board. A team member will be along shortly to respond. To request changes to the docs you can also open a 
|
Hi @MathiasVP, this week's docs content team's first responder here👋🏻 Thanks for ping. I'll add this to our review board and a writer will pick it up for review in their next reviewing session. |
| unsigned max_size = sizeof(buffers->array); | ||
| unsigned free_size = max_size - len_array; | ||
| strncat(buffers->array, s, free_size); // BAD | ||
| } |
There was a problem hiding this comment.
What about the test cases from experimental involving "fix" and MAX_SIZE and len + 1?
There was a problem hiding this comment.
I've added these in e857ac1. Those tests make the query look better than it really is since they're only GOOD because we don't handle the malloc case in the query. It doesn't hurt to have them, though.
There was a problem hiding this comment.
Yeah, there are a few obvious cases missing. If we make improvements to support malloc at some point we'll want to add them.
cpp/ql/src/Likely Bugs/Memory Management/SuspiciousCallToStrncat.ql
Outdated
Show resolved
Hide resolved
This is a weird case - I think their code is safe but buggy, as an overflow causes it to put Results LGTM. This is clearly a fairly common error. |
There was a problem hiding this comment.
@MathiasVP - this LGTM ✨
One typo and 2 other comments for your consideration.
Approving this so my review is not blocking you.
| * @name Potentially unsafe call to strncat | ||
| * @description Calling 'strncat' with the size of the destination buffer | ||
| * as the third argument may result in a buffer overflow. | ||
| * @description Calling `strncat` with the size of the destination buffer as the third argument may result in a buffer overflow. |
There was a problem hiding this comment.
The description is a bit long but happy for you to leave it as is if there's no workaround
There was a problem hiding this comment.
Thanks! Fixed in 78cc8f0.
cpp/ql/src/Likely Bugs/Memory Management/SuspiciousCallToStrncat.qhelp
Outdated
Show resolved
Hide resolved
| <overview> | ||
| <p>The standard library function <code>strncat</code> appends a source string to a target string. | ||
| The third argument defines the maximum number of characters to append and should be less than or equal to the remaining space in the destination buffer. Calls of the form <code>strncat(dest, src, strlen(dest))</code> or <code>strncat(dest, src, sizeof(dest))</code> set the third argument to the entire size of the destination buffer. Executing a call of this type may cause a buffer overflow unless the buffer is known to be empty. Buffer overflows can lead to anything from a segmentation fault to a security vulnerability.</p> | ||
| The third argument defines the maximum number of characters to append and should be less than or equal to the remaining space in the destination buffer. |
There was a problem hiding this comment.
All these sentences are in the same paragraph. Perhaps consider breaking this content into 2 or 3 paragraph to improve readability? Feel free to ignore if you don't agree.
There was a problem hiding this comment.
Oh! Good point. Fixed in 5382ef7.
Co-authored-by: mc <[email protected]>
|
@geoffw0 I believe all comments (including documentation) have been addressed now. |
5 participants
(Fixes https://github.com/github/codeql-c-team/issues/275.)
This PR promotes
cpp/access-memory-location-after-end-buffer-strncatout of experimental by moving the bad case identified in #5009 into thecpp/unsafe-strncatquery. The new case contributes to a couple of new results on the usual LGTM projects: https://lgtm.com/query/6385023132115309042/.It could be argued that the result on
pmacct/pmacctis a false positive as there's a guard that handles the bad case a couple of lines above the place where we raise an alert. I still think it would be in the interest of the programmer to fix it, though.@github/docs-content-codeql, would you please review the
.qhelpand@descriptionchanges and the change-note in this PR?