Make ERR_STATE opaque#9082
Make ERR_STATE opaque#9082richsalz wants to merge 2 commits intoopenssl:masterfrom richsalz:make-errstate-opaque
Conversation
slontis
left a comment
slontis
left a comment
There was a problem hiding this comment.
This looks much cleaner to me.
Would someone be using this structure that is now opaque is the question.
|
New commit pushed, thanks. |
crypto/err/err.c
Outdated
There was a problem hiding this comment.
Is OPENSSL_cleanse() normally used to clear?
|
* Is OPENSSL_cleanse() normally used to clear?
Only for key material.
|
|
Looks good to me.. |
include/openssl/err.h
Outdated
There was a problem hiding this comment.
ERR_get_state is no longer usable with ERR_STATE made opaque. Perhaps just remove it? Debian code search comes up with only the following uses:
- https://sources.debian.org/src/python2.7/2.7.16-2/Modules/_ssl.c/?hl=595#L595
- https://sources.debian.org/src/python3.7/3.7.3-2/Modules/_ssl.c/?hl=909#L909
- https://sources.debian.org/src/myproxy/6.2.4-1/ssl_utils.c/?hl=85#L85
- https://sources.debian.org/src/canl-c/3.0.0-3/src/proxy/sslutils.c/?hl=404#L404
OpenSSL's blog post mentions, for 3.0, that "We plan to keep impacts on existing end user applications to an absolute minimum with the intention that the vast majority of existing well-behaved applications will just need to be recompiled". I think the "well-behaved" qualification covers all of these:
The calls in (1) and (2) are pointless and have already been removed upstream.
(3) and (4) and would be broken by this CL anyway and so are not further negatively impacted by removing the function. (3) should just loop over ERR_get_error rather than reach into OpenSSL's internals. (4) is thoroughly not a well-behaved use of OpenSSL's APIs and would break anyway if OpenSSL were to allocate flag 64. (Also the flag it sets appears to be unused...)
It's used in the fuzzing tests. @kroeckx can you explain why? |
|
Ping for a review, and @kroeckx to explain why the fuzzing calls |
|
weekly bump. :) |
|
Regarding the fuzzers, |
|
ERR_get_state it used to make sure some global variables are
initialized, so that things are reproducible. Otherwise the fuzzer
might think that a change in the file caused it to take a
different path, while the change in file didn't have any impact on
it.
This only useful for fuzzers that run several files in the same
process, like most do for speed reasons. It's not very important,
buf if you can replace it with an other call that has the same
effect that would be great.
|
|
So it sounds like clearing the error state, which will intiailize things, will accomplish the same affect. I’ll try it and see.
Thanks for the info!
|
|
Removed |
|
Additional commit pushed. Not sure if I got the syntax right in err.c file, please check.
|
|
In my haste I forgot to add the include file to the commit. ;( I updated the commit and pushed.
|
|
Sigh. What about the util/missing files?
|
I haven't dived into that, so I have no idea. I'd say that the safest is to restore them... |
|
updated commit that restores all of util pushed. |
So you need another |
|
YAUCP. (yet another updated commit pushed) |
crypto/err/err.c
Outdated
There was a problem hiding this comment.
You could make this static ossl_inline. Entirely your choice, it's fine either way
levitte
added
branch: master
|
So we have answers for all everything that has been found so far. What is the best way forward? Wiki entry? Blog post? |
|
Python's already been fixed for over a year: python/cpython@55e53c3 The myproxy code could just loop over (See also #9082 (comment)) |
Also change the internals of ERR_STATE so that instead of five parallel arrays, it has a single array of struct's and leverage that to make the code simpler. Change flag tests to explicitly test against zero. Move some declarations, downcase static functions. Remove err_ prefix from field names. Remove superfluous prefixes (err_ usually) and suffixes (_int often). Pass in pointers to clear_data, etc. Rewrite set_data, make it void (can't fail)
Call get_state in ERR_get_state() In fuzzing, replace it with ERR_clear_error() which has the same desired side-effect.
|
We have an new issue that affects the decision as well: #9458 The latter can be fixed by simply not allowing the function name to be recorded... some people be sad, though... |
|
Or maybe I'm over-thinking this... new fields aren't quite as bad as a complete restructure or making the structure opaque, perhaps? |
|
Adding fields is not a problem, it is a major number. You can even
add them in the middle of the current struct if you want.
(Is this the answer you're looking for? I'm not sure what the
question really is)
|
|
I want to know if making ERR_STATE opaque is okay, if the OMC needs to vote, or if there is something more concrete that can be done. From the comments here, you seem to be the only OMC member opposed, and I do not know what it will take to resolve the issue for you. |
|
Making it opaque does not seems like a block for those other things, unless I'm missing something. At least the code in voms seems to suggest this is a workaround for ERR_get_error_line_data() not being available on Win32 in ssleay 0.9. I suggest that we at least start by asking them to change their code. I really don't feel like saying it's ok to break those few applications we know about that will be broken. There are probably others. And it's a slippery slope, you seem to want to break a few applications in 1 PR, and then a few others in the next, and so on. |
|
I was thinking a bit more about this, and @kroeckx is quite right that this is not strictly necessary to make ERR_STATE opaque, unless you expect it to change wildly between, say, 3.0 and 4.0. If our rules allow us to add fields until 3.0 is released, I think we're safe. |
|
What we can do is to make sure any missing accessor in in place (if any... I have my doubts), and possibly deprecate the public structure (I have an idea on how to do that), so it's allowed to be made opaque later in 4.0. |
|
I am only asking about this PR, no others. If other PR's are approved before this one, work will need to be done. I am assuming that the other PR's will happen before this is approved. I want to know should I bother to do the work or not. I am a bit frustrated by the state of THIS PR. One OMC member is against it, but hasn't formally putting a -1/hold on it, so I am at a loss for how to come to a decision here. A couple of people have asked, multiple times, if he still has concerns, what it would take to resolve them, and so on. |
|
I am not interested in holding onto this branch until development on 4.0 starts. I will close it if/when @kroeckx gives a definitive NO to this. And then I'll delete the branch a couple of days later. |
|
Actually, it was @kroeckx that added the "hold" and the "4.0" labels. I do not know what more formality you need. I interpret those two labels as saying that this is a good idea, but not now. As for me, I have changed my mind and agree with @kroeckx. The broader message on the wall is that we should be more careful with removals in 3.0. It has been repeated a few times that we should not break well-behaving apps, and I'll admit that I've made a more liberal and less cautious interpretation of "well-behaving" than others. My conclusion is that we should probably look at deprecation before removal, to think of removal as a 4.0 thing if we have deprecated properly before that. See #9462 for how that can be done with ERR_STATE. |
|
Fair enough. I am closing this, and in a couple of days I'll remove the branch. If someone wants to clone it beforehand, feel free of course. |
|
(a point that is lost in the PR description is that this not only makes ERR_STATE opaque, but also restructures it, which is actually quite sane) |
|
Is the restructuring acceptable? The impression I got from the "adding fields" sub-thread is no. |
|
It's not acceptable for 3.0, no. That says nothing about the future beyond, though... |
|
I understand you don't want to do this work for 3.0 Someone else will have to pick it up and move it forward for 4.0 and clone the branch. |
|
squashed, fixed commentary, made it simpler for someone else to pick this up. |
|
Maybe you should push, too 😉 |
This comment was marked as abuse.
This comment was marked as abuse.
|
I think I've been very clear, but in case you've missed it:
- I've put the "hold" label on it. You can consider that a -1.
- Nobody has disagreed with that, or tried to put it to a vote.
- If you want to make this happen at some point, you should
deprecate the structure and functions, so we can do it in the
next version.
|
|
Consider a hold as -1: okay, that makes sense. I'm going to stay with the actions I said above. |
6 participants
I don't know if the OMC wants to do this, but I expect it will be "safe"
This is part of a set of ERR-related changes I've been proposing (dare I say improvements?) but like the others is independent of each.