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

infof: clearly indicate truncation #3216

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@danielgustafsson
Member

danielgustafsson commented Nov 1, 2018

It was reported off-list that very large error messages garbled the output due to infof() truncating and losing the trailing newline. This attempts to handle that gracefully, and also include a more clear indication that the output was indeed truncated.

The internal buffer in infof() is limited to 2048 bytes of payload plus an additional byte for NULL termination. Servers with very long error messages can however cause truncation of the string, which currently isn't very clear, and leads to badly formatted output.

This appends a ... marker to the end of the string to clearly show that it has been truncated, and adds a mandatory newline. There are cases when infof() input doesn't require a newline, but when there is truncation of the input is probably not one of them so forcibly add it to avoid breaking the output.

Also include a unittest covering infof() to try and catch any bugs introduced in this quite important function.

infof: clearly indicate truncation
The internal buffer in infof() is limited to 2048 bytes of payload plus
an additional byte for NULL termination. Servers with very long error
messages can however cause truncation of the string, which currently
isn't very clear, and leads to badly formatted output.

This appends a "..." marker to the end of the string to clearly show
that it has been truncated, and adds a mandatory newline. There are
cases when infof() input doesn't require a newline, but when there is
truncation of the input is probably not one of them so forcibly add
it to avoid breaking the output.

Also include a unittest covering infof() to try and catch any bugs
introduced in this quite important function.
* | (__| |_| | _ <| |___
* \___|\___/|_| \_\_____|
*
* Copyright (C) 1998 - 2016, Daniel Stenberg, <daniel@haxx.se>, et al.

This comment has been minimized.

@danielgustafsson

danielgustafsson Nov 1, 2018

Member

Clearly I didn't read it carefully enough when I copy pasted, will fix the year before pushing in case this goes in.

lib/sendf.c Outdated
vsnprintf(print_buffer, sizeof(print_buffer), fmt, ap);
len = vsnprintf(print_buffer, sizeof(print_buffer), fmt, ap);
if(len >= sizeof(print_buffer))
snprintf(print_buffer + (sizeof(print_buffer) - 5), 5, "...\n");

This comment has been minimized.

@danielgustafsson

danielgustafsson Nov 1, 2018

Member

I'm not a fan of magic numbers, but the alternatives didn't seem terribly appealing either. Any better suggestions?

This comment has been minimized.

@bagder

bagder Nov 1, 2018

Member

Not a fan either, but in this case the plain number seems like the better choice.

@bagder

bagder approved these changes Nov 1, 2018

@bagder

This comment has been minimized.

Member

bagder commented Nov 2, 2018

Something else: this assumes the string is newline terminated if too long, as it adds one then. Can we be sure of this?

@danielgustafsson

This comment has been minimized.

Member

danielgustafsson commented Nov 2, 2018

Something else: this assumes the string is newline terminated if too long, as it adds one then. Can we be sure of this?

I was thinking that there are no legitimate usecases of a truncated string not having a trailing \n, but the more I think about the less reasonable it seems. Curl_infof() could however inspect the passed format and only append the \n in case the format has one, that should be easy enough it seems.

@bagder

This comment has been minimized.

Member

bagder commented Nov 2, 2018

Curl_infof() could however inspect the passed format and only append the \n in case the format has one

Yes. It's either that, or we make a separate function entry for non-trailing-newline use, as they're very rare in the code base.

@bagder

bagder approved these changes Nov 2, 2018

@bagder

This comment has been minimized.

Member

bagder commented Nov 2, 2018

The appveyor failure is unrelated - I didn't bother to retrigger those builds.

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