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

Jump to bottom

Fix format strings #2561

Closed
wants to merge 5 commits into from
Closed

Fix format strings #2561

wants to merge 5 commits into from

Conversation

rikardfalkeborn
Copy link
Contributor

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
Copy link
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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
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 Teach GCC (and Clang) to diagnose curl_m*printf format strings Fix format strings May 12, 2018
@bagder
Copy link
Member

bagder commented May 14, 2018

thanks!

@bagder bagder closed this in 13505dc May 14, 2018
@rikardfalkeborn rikardfalkeborn deleted the format-specifiers branch May 14, 2018 16:11
@rikardfalkeborn rikardfalkeborn mentioned this pull request May 30, 2018
Closed
@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.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@rikardfalkeborn @bagder