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

Fix format strings #2561

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
2 participants
@rikardfalkeborn
Contributor

rikardfalkeborn commented May 11, 2018

This is a series of commits cleaning up format specifiers for the curl_*printf functions as a preparation for the last commit where an attribute is added to show which arguments are the format string, and which are the format specifiers.

A few comments on some of the commits:

tests: Fix format specifiers - In unit1323.c, the exact types for the members of the timeval struct is not specified, perhaps I should have added a cast to avoid protential problems on other platforms?

Make sure format length is int - Apparently, the standard says field precision specifiers should be of type int. I just added a cast to the variables. I have no idea if there's a risk it will overflow or wrap...

Add attribute to teach Gcc about printf-functions - A big bummer is that curl printf extensions (such as %.-2f) now gives warnings). I was not able to figure out if there was a way tell gcc about these extensions. It also means anyone using curl/mprintf.h with nonstandard extensions will get a warning.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 11, 2018

Member

This pull request introduces 2 alerts when merging 1dce353 into c3d7db4 - view on lgtm.com

new alerts:

  • 2 for Wrong type of arguments to formatting function

Comment posted by lgtm.com

Member

bagder commented May 11, 2018

This pull request introduces 2 alerts when merging 1dce353 into c3d7db4 - view on lgtm.com

new alerts:

  • 2 for Wrong type of arguments to formatting function

Comment posted by lgtm.com

@rikardfalkeborn

This comment has been minimized.

Show comment
Hide comment
@rikardfalkeborn

rikardfalkeborn May 11, 2018

Contributor

No surprise CI found a number of issues I didn't spot for different configurations/etc. I'll try and fix these and update the PR.

Contributor

rikardfalkeborn commented May 11, 2018

No surprise CI found a number of issues I didn't spot for different configurations/etc. I'll try and fix these and update the PR.

@rikardfalkeborn

This comment has been minimized.

Show comment
Hide comment
@rikardfalkeborn

rikardfalkeborn May 11, 2018

Contributor

It seems travis builds spews out warnings about ISO C90 does not support the ‘z’ gnu_printf length modifier which curl/lib/mprintf.c does support. I suppose I should drop the attribute commit (and the format specifier one). And perhaps even split the PR in smaller ones?

Contributor

rikardfalkeborn commented May 11, 2018

It seems travis builds spews out warnings about ISO C90 does not support the ‘z’ gnu_printf length modifier which curl/lib/mprintf.c does support. I suppose I should drop the attribute commit (and the format specifier one). And perhaps even split the PR in smaller ones?

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 11, 2018

Member

Thanks for your work!

It seems travis builds spews out warnings about ISO C90 does not support the ‘z’ gnu_printf length modifier which curl/lib/mprintf.c does support.

Yeah, I was afraid exactly something like this would happen. The curl printf() code is not exactly like the C90 one.

I suppose I should drop the attribute commit (and the format specifier one).

I'm afraid I think that might be the only solution that doesn't force us to do other ugly things to avoid those warnings. Your work still identified a number of improvements thanks to that!

And perhaps even split the PR in smaller ones?

Nah, I'm fine with having "fixing printf() modifiers" being a single one etc.

Member

bagder commented May 11, 2018

Thanks for your work!

It seems travis builds spews out warnings about ISO C90 does not support the ‘z’ gnu_printf length modifier which curl/lib/mprintf.c does support.

Yeah, I was afraid exactly something like this would happen. The curl printf() code is not exactly like the C90 one.

I suppose I should drop the attribute commit (and the format specifier one).

I'm afraid I think that might be the only solution that doesn't force us to do other ugly things to avoid those warnings. Your work still identified a number of improvements thanks to that!

And perhaps even split the PR in smaller ones?

Nah, I'm fine with having "fixing printf() modifiers" being a single one etc.

@rikardfalkeborn rikardfalkeborn changed the title from Teach GCC (and Clang) to diagnose curl_m*printf format strings to Fix format strings May 12, 2018

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 14, 2018

Member

thanks!

Member

bagder commented May 14, 2018

thanks!

@bagder bagder closed this in 13505dc May 14, 2018

@rikardfalkeborn rikardfalkeborn deleted the rikardfalkeborn:format-specifiers branch May 14, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Aug 12, 2018

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