Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mprintf: fix integer handling in float precision #15988

Closed
wants to merge 1 commit into from

Conversation

bagder
Copy link
Member

@bagder bagder commented Jan 13, 2025

In the double output function when an extremely large width and precision is set that reaches the libcurl maximum (325), the handling of the precision part would do wrong which could lead to bad output.

Extend test 557 to verify.

Coverity CID 1638751.

@github-actions github-actions bot added the tests label Jan 13, 2025
@testclutch

This comment was marked as outdated.

@bagder bagder force-pushed the bagder/mprintf-precision branch from 8e7f89d to 5ba187c Compare January 13, 2025 13:35
@bagder
Copy link
Member Author

bagder commented Jan 13, 2025

There is a single CI job failure for test 557 that reproduces for this PR right now. The job is Windows / old-mingw, CM 7.3.0-x86_64 schannel U.

The failure in test 557 is a call to curl_msnprintf() writing a formatted double to a buffer. The test outputs it with a large width and precision, like this:

rc = curl_msnprintf(larger, sizeof(larger), "%326.326f", 0.1);

Internally, in lib/mprintf.c this makes libcurl call the real snprintf() implementation. It does this for floating point outputs. It calls snprintf() with a local stack based buffer with a generated format string.

Like this:

#define BUFFSIZE 326
(snprintf)(work, BUFFSIZE, formatbuf, iptr->val.dnum); /* NOLINT */

With my new tests and for debugging my case here, I added the following logic below the snprintf call:

if(strlen(work) >= BUFFSIZE) {
  (fprintf)(stderr, "Nasty size: %zd\n", strlen(work));
}

This condition should never execute because snprintf() is obliged to (I quote my man page)

write at most size bytes (including the terminating null byte ('\0')) to str.

doing strlen() on the target buffer after snprintf is called should always be smaller than the size argument given to snprintf, since the null terminator is not counted in strlen() but should be stored in the buffer.

And yet, in the failing job I get this output:

 Nasty size: 326
 Nasty size: 326
 Nasty size: 326

(From my three invocations outputting wide floating point numbers)

In the double output function when an extremely large width and
precision is set that reaches the libcurl maximum (325), the handling of
the precision part would do wrong which could lead to bad output.

Also: work-around for single-byte buffer snprintf overflow with mingw.

Extend test 557 to verify.

Coverity CID 1638751.

Closes #15988
@bagder bagder force-pushed the bagder/mprintf-precision branch from 0a73179 to 9866e88 Compare January 13, 2025 18:38
@bagder bagder closed this in 7e32f65 Jan 13, 2025
@bagder bagder deleted the bagder/mprintf-precision branch January 13, 2025 22:42
@jay
Copy link
Member

jay commented Jan 14, 2025

The mingw issue is really a CRT issue, it's not guaranteed on Windows the snprintf will always be null terminated unless you use the C99 compliant function which is only in later CRTs. See #15997 to fix it for all Windows.

Also- I didn't know we were still supporting old mingw? @vszakats

@vszakats
Copy link
Member

vszakats commented Jan 14, 2025

The mingw issue is really a CRT issue, it's not guaranteed on Windows the snprintf will always be null terminated unless you use the C99 compliant function which is only in later CRTs. See #15997 to fix it for all Windows.

Also- I didn't know we were still supporting old mingw? @vszakats

We are not, I just had to make the name short to fit on the screen; full name is old-mingw-w64. Just before reading this, I made a local commit to change it to old-mingw64 in my cegcc branch (will push later). Sorry for the confusion!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

4 participants