Fix warning for possible string truncation#2790
Merged
andypugh merged 3 commits intoLinuxCNC:2.9from Dec 14, 2023
Merged
Conversation
Collaborator
|
How many does this catch? I have notice that the compile log is sprinkled with hundreds of string truncation warnings. |
Contributor
Author
|
On 13.12.2023 17:13, andypugh wrote:
How many does this catch? I have notice that the compile log is sprinkled with hundreds of string truncation warnings.
Only three, it was all from that file. If you are satisfied with
how i solved it, I can see if I can't fix some more of them, but not
hundreds.
I forgot to check if they are present in 2.9, should i target any
specific branch?
|
Collaborator
|
There are many truncation warnings when compiling 2.9. So I would suggest targetting that. I probably won't merge them until after 2.9.2 though. |
All of these fixes handles strncpy(), and the compiler warning have been silenced by checking the length before copying the string, we check with '>=' which means that the string must be one smaller and therefore have room for at least one NULL terminating character, if this fails the function provides a error message and/or return. Some of the buffer lengths had a '+ 1' appended, some of these have been removed. I'm also using sizeof() operator, in case we later want to change the buffer size. Most of the previous solutions also checked for NULL terminated strings but the compiler wasn't satisfied with it happening *after* the command to strncpy().
This commit changes strncpy() for snprintf(), snprintf() guarantees a NULL terminating string. These places did not check the length at all, some hard-coded a NULL terminating string as the last place in the array. With this, some of the strings could potentially be without the NULL terminating character. By changing to snprintf() we run into the possibility of truncating the strings, no checks is currently in place to catch this. There is also a strncat() thrown on here, hard-coded value was changed from 1 to 2 to make room for the NULL terminating character.
Increased from 80 to 256, this buffer is used when creating the stack trace used within LinuxCNC. It is used quite extensible throughout the application.
havardAasen
force-pushed
the
fix-string-truncation
branch
from
d5b3cd1 to
7f54de8
Compare
Contributor
Author
|
This fixes quite a lot of the warning for string truncation. The one responsible for most of them was fixed with 7f54de8. |
Collaborator
|
I changed my mind, there was quite a backlog of 2.9 fixes which I am working through. This might as well go in too. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We check the length of the string before using
strncpy(), we check with>=which means that the string must be one smaller and therefore have room for at least one NULL terminating character.The previous solution also checked for NULL terminated strings but the compiler wasn't satisfied with it happening after the command to
strncpy().For the error message, the unnecessary cast to unsigned long is removed, and the format string has been changed to
zu.