deprecate sprint and replace with snprintf #4494
Merged
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
This comment was marked as resolved.
This comment was marked as resolved.
|
I'll fix if I can get succesful tests. I'm also still missing one sprintf. (Disclaimer; I don't have any OSX machines so I'm really dependent on CI) Not sure what's happening with CI tests now as I'm hardly getting any output. I might still be having the max size wrong though |
|
Not sure why it says it's not successful. All tests seem fine? |
there are some failures, but it's just the usual flaky tests |
|
Ok, but where do you see those failures. I only see green checkmarks |
|
You don't see these in the list?
|
|
Aargh, my browser is hiding the scrollbar so I never noticed I can scroll down. |
daira
reviewed
Feb 1, 2023
| @@ -129,7 +129,8 @@ static std::string make_address_string (const char *hbuf_, | |||
| pos += hbuf_len; | |||
| memcpy (pos, ipv6_suffix_, sizeof ipv6_suffix_ - 1); | |||
| pos += sizeof ipv6_suffix_ - 1; | |||
| pos += sprintf (pos, "%d", ntohs (port_)); | |||
| pos += snprintf (pos, max_port_str_length + 1 * sizeof (char), "%d", | |||
There was a problem hiding this comment.
The return value of snprintf is the "number of characters (not including the terminating null character) which would have been written to buffer if bufsz was ignored, or a negative value if an encoding error (for string and character conversion specifiers) occurred". (Arguably this was a misdesign of snprintf.) So if there were a buffer overflow, pos would be calculated incorrectly and the call to the std::string constructor below would still be undefined behaviour.
A better way is to either use snprintf_s (requires __STDC_WANT_LIB_EXT1__ to be defined before #include <cstdio>, and C++11), or add an explicit zmq_assert. I will file a PR shortly, covering all the snprintf calls in this PR.
Less importantly, sizeof(char) == 1 by definition, so it's pointless (and makes the code no clearer) to multiply by it.
daira
added a commit
to daira/libzmq
that referenced
this pull request
Feb 1, 2023
…at snprintf can truncate, and then return the number of characters that would have been written without truncation. Replace such calls with calls to a zmq_snprintf helper that asserts in the case of error or truncation. Signed-off-by: Daira Hopwood <[email protected]>
daira
added a commit
to daira/libzmq
that referenced
this pull request
Feb 1, 2023
…at snprintf can truncate, and then return the number of characters that would have been written without truncation. Replace such calls with calls to a zmq_snprintf helper that asserts in the case of error or truncation. Signed-off-by: Daira Hopwood <[email protected]>
daira
added a commit
to daira/libzmq
that referenced
this pull request
Feb 1, 2023
…at snprintf can truncate, and then return the number of characters that would have been written without truncation. Replace such calls with calls to a zmq_snprintf helper that asserts in the case of error or truncation. Signed-off-by: Daira Hopwood <[email protected]>
daira
added a commit
to daira/libzmq
that referenced
this pull request
Feb 1, 2023
…at snprintf can truncate, and then return the number of characters that would have been written without truncation. Replace such calls with calls to a zmq_snprintf helper that asserts in the case of error or truncation. Signed-off-by: Daira Hopwood <[email protected]>
daira
added a commit
to daira/libzmq
that referenced
this pull request
Feb 1, 2023
…at snprintf can truncate, and then return the number of characters that would have been written without truncation. Replace such calls with calls to a zmq_snprintf helper that asserts in the case of error or truncation. Signed-off-by: Daira Hopwood <[email protected]>
daira
added a commit
to daira/libzmq
that referenced
this pull request
Feb 1, 2023
…at snprintf can truncate, and then return the number of characters that would have been written without truncation. Replace such calls with snprintf_s. Signed-off-by: Daira Hopwood <[email protected]>
daira
added a commit
to daira/libzmq
that referenced
this pull request
Feb 1, 2023
…at snprintf can truncate, and then return the number of characters that would have been written without truncation. Replace such calls with snprintf_s. Signed-off-by: Daira Hopwood <[email protected]>
daira
added a commit
to daira/libzmq
that referenced
this pull request
Feb 1, 2023
…at snprintf can truncate, and then return the number of characters that would have been written without truncation. Replace such calls with snprintf_s. Signed-off-by: Daira Hopwood <[email protected]>
daira
added a commit
to daira/libzmq
that referenced
this pull request
Feb 1, 2023
…at snprintf can truncate, and then return the number of characters that would have been written without truncation. Signed-off-by: Daira Hopwood <[email protected]>
daira
added a commit
to daira/libzmq
that referenced
this pull request
Feb 1, 2023
…at snprintf can truncate, and then return the number of characters that would have been written without truncation. Signed-off-by: Daira Hopwood <[email protected]>
daira
added a commit
to daira/libzmq
that referenced
this pull request
Feb 1, 2023
…at snprintf can truncate, and then return the number of characters that would have been written without truncation. Signed-off-by: Daira Hopwood <[email protected]>
daira
added a commit
to daira/libzmq
that referenced
this pull request
Feb 1, 2023
…at snprintf can truncate, and then return the number of characters that would have been written without truncation. Signed-off-by: Daira Hopwood <[email protected]>
nuttycom
added a commit
to nuttycom/zcash
that referenced
this pull request
Feb 1, 2023
@daira identified an error in zeromq/libzmq#4494 that we reproduced in zcash#6393. This adds @daira's patch from zeromq/libzmq#4507 to ensure we handle this potential case of undefined behavior. Co-authored-by: Daira Hopwood <[email protected]>
bluca
added a commit
that referenced
this pull request
Feb 1, 2023
#4494 added calls to snprintf, but did not take into account that snprintf can truncate
drgora
pushed a commit
to HorizenOfficial/zen
that referenced
this pull request
Apr 12, 2023
@daira identified an error in zeromq/libzmq#4494 that we reproduced in zcash/zcash#6393. This adds @daira's patch from zeromq/libzmq#4507 to ensure we handle this potential case of undefined behavior. Co-authored-by: Daira Hopwood <[email protected]>
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.
Problem: Latest OSX deprecates using sprint resulting is failure to build
Fix by switching to snprint.
As tried by #4481 and fixing #4478