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

Use of snprintf and vsnprintf in Curl #3296

Closed
thoger opened this Issue Nov 21, 2018 · 2 comments

Comments

Projects
None yet
2 participants
@thoger
Contributor

thoger commented Nov 21, 2018

The curl code base use snprintf or vsnprintf functions on multiple places. However - just like for all printf family functions - it uses its own implementation of those functions - curl_msnprintf and curl_mvsnprintf - which is achieved via macro definitions in curl_printf.h:

https://github.com/curl/curl/blob/curl-7_62_0/lib/curl_printf.h

However, there is a notable difference in what curl_m*snprintf functions return compared to what's defined for *snprintf in POSIX or C99 for the case when destination buffer is too small and the output is truncated:

  • curl functions return the number of characters written, which is also documented in https://curl.haxx.se/libcurl/c/curl_mprintf.html
  • POSIX requires the number of characters that should have been written if the destination buffer was sufficiently large

Looking at few cases where snprintf return value is not ignored, the curl code expects and relies on the curl implementation return values. There are often no checks to ensure that the return value does not exceed destination buffer size, and the value is directly used to increment the pointer that is used for subsequent writes, or decrement free space counter. That could lead to out-of-bounds buffer access and integer underflow if POSIX snprintf was used instead. An example of such code pattern can be found in e.g. curl_version():

https://github.com/curl/curl/blob/curl-7_62_0/lib/version.c#L129-L131

This can be a problem if *snprintf implementation used by curl is to be changed in the future, or if anyone tries to make curl use printf functions from their system C library instead of curl implementations.

This can also cause confusion to folks from outside of the Curl project who review or contribute to the code of the project.

The explicit use of curl_m*snprintf functions should be considered for places where non-standard return values are expected.

@bagder

This comment has been minimized.

Member

bagder commented Nov 21, 2018

Since the function returns another return value I think I would prefer to change the function name in all places so that we can check it and fail if the wrong function name is used when code gets changed in the future and possibly start to use the return code. I'll file a PR.

bagder added a commit that referenced this issue Nov 21, 2018

snprintf: renamed and we now only use msnprintf()
The function does not return the same value as snprintf() normally does,
so readers may be mislead into thinking the code works differently than
it actually does. A different function name makes this easier to detect.

This also adds snprintf() and vsnprintf() is "banned" functions in
checksrc to make us not mistakenly use them going forward.

Reported-by: Tomas Hoger
Assisted-by: Daniel Gustafsson
Fixes #3296

bagder added a commit that referenced this issue Nov 22, 2018

snprintf: renamed and we now only use msnprintf()
The function does not return the same value as snprintf() normally does,
so readers may be mislead into thinking the code works differently than
it actually does. A different function name makes this easier to detect.

Reported-by: Tomas Hoger
Assisted-by: Daniel Gustafsson
Fixes #3296

bagder added a commit that referenced this issue Nov 22, 2018

snprintf: renamed and we now only use msnprintf()
The function does not return the same value as snprintf() normally does,
so readers may be mislead into thinking the code works differently than
it actually does. A different function name makes this easier to detect.

Reported-by: Tomas Hoger
Assisted-by: Daniel Gustafsson
Fixes #3296
Closes #3297
@thoger

This comment has been minimized.

Contributor

thoger commented Nov 22, 2018

One more observation regarding curl_msnprintf() - if the output buffer is large enough, the return value is the number of characters written, excluding the terminating nul byte, however, when output is truncated, the terminating nul byte is counted in the return value.

Repeating the test from:

http://demin.ws/blog/english/2013/01/28/use-snprintf-on-different-platforms/

for curl, the output is:

capacity=0, n=0, buf=[abcdefghijk] (length 11)
capacity=1, n=1, buf=[] (length 0)
capacity=2, n=2, buf=[1] (length 1)
capacity=3, n=3, buf=[12] (length 2)
capacity=4, n=3, buf=[123] (length 3)
capacity=5, n=3, buf=[123] (length 3)

@bagder bagder closed this in dcd6f81 Nov 23, 2018

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