ZOOKEEPER-3079: avoid unsafe use of sprintf(3) #559
Closed
Conversation
This file contains 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
The function format_endpoint_info declares both addrstr and buf as 128 element char arrays, however on non-Windows platforms it calls sprintf(3) to write into buf the value of addrstr followed by ':' followed by the the port number. This causes a compiler error when building with GCC 8 because this could potentially overflow buf if the value of addrstr was ever 127 characters long (or a little less depending on how many digits are in port). Of course, this couldn't actually happen because addrstr is initialized by inet_ntop(3) which won't write more than INET6_ADDRSTRLEN bytes (defined in <netinet/in.h> on POSIX-compliant systems). Of course, GCC doesn't know that, so let's just declare addrstr as a char array of only size INET6_ADDRSTRLEN instead of 128. Signed-off-by: Kent R. Spillner <[email protected]>
nkalmar
reviewed
Jul 4, 2018
| @@ -4357,7 +4357,7 @@ int zoo_add_auth(zhandle_t *zh,const char* scheme,const char* cert, | |||
| static const char* format_endpoint_info(const struct sockaddr_storage* ep) | |||
| { | |||
| static char buf[128] = { 0 }; | |||
| char addrstr[128] = { 0 }; | |||
| char addrstr[INET6_ADDRSTRLEN] = { 0 }; | |||
There was a problem hiding this comment.
Isn't this by default 46 as per POSIX? I don't see this overridden in ZK.
There was a problem hiding this comment.
Check and check, it is 46 and ZK does not override it. I apologize if my wording wasn't clear enough, but the issue that GCC 8 errors on is the fact that buf and addrstr are both size 128, but later on in the call to sprintf(3) we write addrstr + ':' + port into buf. GCC 8 sees that buf & addrstr are both potentially 128 characters long, so the sprintf(3) could potentially overflow buf when it tacks on the colon & port. Of course, we know that will never happen, but GCC 8 doesn't. So by resizing the declaration of addrstr we safely avoid any confusion or doubt.
There was a problem hiding this comment.
OK, thanks for clearing that up. Well, you were clear now that I understand the issue - probably the problem was that I'm not much of a C developer :)
|
+1 thanks for the submission. if there are no objections, i'll commit this tomorrow |
asfgit
pushed a commit
to apache/mesos
that referenced
this pull request
Aug 21, 2018
This is a backport of: apache/zookeeper#559 Review: https://reviews.apache.org/r/68370/
asfgit
pushed a commit
to apache/mesos
that referenced
this pull request
Aug 21, 2018
This is a backport of: apache/zookeeper#559 Review: https://reviews.apache.org/r/68370/
asfgit
pushed a commit
to apache/mesos
that referenced
this pull request
Aug 21, 2018
This is a backport of: apache/zookeeper#559 Review: https://reviews.apache.org/r/68370/
asfgit
pushed a commit
to apache/mesos
that referenced
this pull request
Aug 21, 2018
This is a backport of: apache/zookeeper#559 Review: https://reviews.apache.org/r/68370/
asfgit
pushed a commit
to apache/mesos
that referenced
this pull request
Aug 21, 2018
This is a backport of: apache/zookeeper#559 Review: https://reviews.apache.org/r/68370/
Gilbert88
pushed a commit
to d2iq-archive/mesos
that referenced
this pull request
Sep 28, 2018
This is a backport of: apache/zookeeper#559 Review: https://reviews.apache.org/r/68370/
RokLenarcic
pushed a commit
to RokLenarcic/zookeeper
that referenced
this pull request
Sep 3, 2022
The function format_endpoint_info declares both addrstr and buf as 128 element char arrays, however on non-Windows platforms it calls sprintf(3) to write into buf the value of addrstr followed by ':' followed by the the port number. This causes a compiler error when building with GCC 8 because this could potentially overflow buf if the value of addrstr was ever 127 characters long (or a little less depending on how many digits are in port). Of course, this couldn't actually happen because addrstr is initialized by inet_ntop(3) which won't write more than INET6_ADDRSTRLEN bytes (defined in <netinet/in.h> on POSIX-compliant systems). Of course, GCC doesn't know that, so let's just declare addrstr as a char array of only size INET6_ADDRSTRLEN instead of 128. Signed-off-by: Kent R. Spillner <kspillneracm.org> Author: Kent R. Spillner <[email protected]> Reviewers: Benjamin Reed <[email protected]> Closes apache#559 from sl4mmy/zookeeper-3079
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The function format_endpoint_info declares both addrstr and buf as 128
element char arrays, however on non-Windows platforms it calls
sprintf(3) to write into buf the value of addrstr followed by ':'
followed by the the port number. This causes a compiler error when
building with GCC 8 because this could potentially overflow buf if the
value of addrstr was ever 127 characters long (or a little less
depending on how many digits are in port). Of course, this couldn't
actually happen because addrstr is initialized by inet_ntop(3) which
won't write more than INET6_ADDRSTRLEN bytes (defined in <netinet/in.h>
on POSIX-compliant systems). Of course, GCC doesn't know that, so let's
just declare addrstr as a char array of only size INET6_ADDRSTRLEN
instead of 128.
Signed-off-by: Kent R. Spillner [email protected]