Join GitHub today
GitHub is home to over 36 million developers working together to host and review code, manage projects, and build software together.
Sign upExpect POSIX compatible vsnprintf. #8788
Conversation
KA101
self-assigned this
Sep 3, 2014
KA101
merged commit 26bdad6
into
CleverRaven:master
Sep 3, 2014
1 check passed
default
Details
BevapDin
deleted the
BevapDin:crash-in-string-format
branch
Sep 3, 2014
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.
BevapDin commentedSep 2, 2014
Fixes #8784
This seems to be a mingw problem. I can reproduce this with the windows-SDL and windows-curses build using wine on linux. It does not happen with the linux build.
My guess:
vstring_format(output.cpp) usesvsnprintfto format the string. That function is supposed to return -1 if the buffer it to small (see http://msdn.microsoft.com/en-us/library/1kt27hek.aspx), in that case the buffer is increased and the function is called again until it succeeds. The function returns a number >= 0 only when the buffer was large enough.The linux/POSIX version of that function returns the size of the buffer that is required to print the formatted string (even when the current buffer size if to small).
It's a bit unclear to me which version mingw uses. There is code in output.cpp that handles both versions (depending on whether it is compiled for window or not). It also depends on localization: with
-DLOCALIZE(which seems to be the default),vsnprintfis actuallylibintl_vsnprintf(coming from libintl, which is part of gettext, it may or may not be POSIX compatible, that apparently depends on its input, see http://lists.gnu.org/archive/html/bug-gnu-utils/2007-02/msg00058.html).The problem boils down to this scenario:
The code assumes
vsnprintfis the windows version (returns -1 when the buffer is too small).The initial buffer size is 1024 bytes, large enough for most strings. The text with the recipes is probably larger, therefor the first call to
vsnprintffails - the windows version would return -1, the buffer would be increased, the function called again. This does not happen.Instead
vsnprintfseems to be the POSIX version and returns the required buffer size (for example 1040). The program assumes that formatting was successful (any value >= 0 indicates success) and that the buffer was large enough. It copies the content of the buffer, but it assumes that the returned value is the length of the string in the buffer (in this case 1040). But the buffer is only 1024 bytes large, so it reads data from outside of the buffer.I've added a simple check for this scenario: if
vsnprintfreturns a number >= 0, the number is compared to the buffer size. If it is less than (or equal to) the buffer size, it is assumed the formatted string has been successfully written in the buffer (and the buffer was large enough) and the buffers content is returned. This works with both windows and POSIX version ofvsnprintf.If the returned number is greater than the buffer size, it is assumed that this is the required buffer size, the buffer is enlarged to that size and
vsnprintfis called again. This indicates a POSIX version ofvsnprintf. The windows version would not return a number greater than the buffer size.If the returned value is negative, it is assumed that the buffer was not large enough, the buffer is enlarged (its size if doubled), and
vsnprintfis called again. This is the same behavior as before.