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

ssl: fix format specifier for size_t argument to BIO_printf #14521

Closed
wants to merge 1 commit into from

Conversation

@paulidale
Copy link
Contributor

@paulidale paulidale commented Mar 11, 2021

Fixes #14519

@t-j-h
t-j-h approved these changes Mar 11, 2021
ssl/t1_enc.c Outdated Show resolved Hide resolved
@paulidale paulidale force-pushed the paulidale:ming-fix branch from 3a75e92 to e7d4434 Mar 11, 2021
@levitte
Copy link
Member

@levitte levitte commented Mar 12, 2021

Do note that MSVC older than 2015 doesn't know %z

@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Mar 12, 2021

Do note that MSVC older than 2015 doesn't know %z

Does MSVC throw errors if it encounters it?
Our BIO_printf knows about it.

@levitte
Copy link
Member

@levitte levitte commented Mar 12, 2021

Does MSVC throw errors if it encounters it?

If compiled with /WX (this corresponds to gcc's -Werror)

@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Mar 12, 2021

I guess the universal fix would be to cast it to a known int type. Not sure I like this given we've got the z modifier.

@levitte
Copy link
Member

@levitte levitte commented Mar 12, 2021

I was a bit confounded when I mentioned MSVC, I'm currently unsure that we ask it to check for format spec mismatches, so it's possible that this will fly unnoticed.

Something to keep in mind, though, is that %z is C99, and we still proclaim that we are sticking to C90 for the language syntax, and while %z is handled by a library function (our own, to boot), we're still asking the compiler (at least gcc and clang) to check these format, which makes it a syntax problem.

The proper universal fix would have been to use PRI macros (from <inttypes.h>)... except that such macros for size_t doesn't exist. However, that has never stopped us from rolling our own when needed, so an idea is to implement a macro PRIz with the appropriate format string (without percent).

@t8m
Copy link
Member

@t8m t8m commented Mar 12, 2021

However we already use %zu throughout the code.

@openssl-machine
Copy link

@openssl-machine openssl-machine commented Mar 12, 2021

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Mar 13, 2021

Merged to master, thanks for the feed back.

@paulidale paulidale closed this Mar 13, 2021
openssl-machine pushed a commit that referenced this pull request Mar 13, 2021
Fixes #14519

Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14521)
@paulidale paulidale deleted the paulidale:ming-fix branch Mar 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants