Harden adding error text. - #32562
Draft
bob-beck wants to merge 20 commits into
Draft
Conversation
Copy link
Copy Markdown
Contributor
Sorry, something went wrong.
ERR_set_error() and ERR_vset_error() take a printf(3) style format string, but carried no format attribute, so nothing verified the arguments supplied at ERR_raise_data() call sites. Add the attribute so we keep this clean. Upon adding the attribute four calls showed up passing a non-literal string as the format, three of them the return value of gai_strerror(). Pass the string as an argument to "%s" instead.
Each of these sites raised an error and then immediately appended a single string to it. ERR_raise_data() does both in one call, and its arguments are checked by the compiler. No functional change.
poll_for_response() formatted the offending value into a local buffer and appended it to the error it had just raised. ERR_raise_data() does both in one call, with the format checked by the compiler. No functional change.
Eleven error paths raised a reason and jumped to bad_str, whose only job was to append the offending string to whichever error had just been raised. ERR_raise_data() carries the string with the reason. No functional change.
Appends printf(3) formatted text to the data of the most recent error, as ERR_add_error_data() does, without the hand-written argument count.
bob-beck
force-pushed
the
err_add_error_vdata_considered_harmful
branch
4 times, most recently
from
f495c02 to
c9af5d2
Compare
August 28, 2026 15:00
It becomes an inline function.
ossl_X509v3_conf_err() replaces the internal uses of the public X509V3_conf_err() macro, which is left as it is.
I should not have put it in internal/err.h
bob-beck
force-pushed
the
err_add_error_vdata_considered_harmful
branch
from
c9af5d2 to
8ff9f27
Compare
August 28, 2026 15:09
When the private key length is not what we expect don't print the public key length.
bob-beck
force-pushed
the
err_add_error_vdata_considered_harmful
branch
2 times, most recently
from
7ab746a to
6d6c16c
Compare
August 28, 2026 15:25
bob-beck
force-pushed
the
err_add_error_vdata_considered_harmful
branch
from
6d6c16c to
b541554
Compare
August 28, 2026 15:38
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I really loved the 90's - the music was great, I was bulletproof, my middle name was DANGER, as
were most of the public API's in C code.
Sadly I'm now much older and feel a lot less bulletproof and tolerant of danger. 35 years of
C programming and surviving a head on collision does stuff to you.
Let's mark the printf style error appenders with the printf attribute, and then fix the things
where we were sort of abusing them. This includes a large sweep at the end to catch everything
in CI I couldn't directly catch. Of note one bug was noticed while doing this where we printed
a public key length when erroring about a private key.
Then there is ERR_add_error_data() and ERR_add_error_vdata() - Back in the 90's
public API that that bravely walked a va_list with an integer provided length into danger without using the usual "stop at a terminating NULL" was just fine (it was the best of times). Today this just seems to be a footgun waiting to happen.
Let's deprecate these strange varargs "append a string" functions, and use an internal replacement
that is safer since it just uses printf. I dont' believe it needs to be public as external callers don't really
need this, they can build up an error string themselves and add it with the existing public api.
Checklist