Use ERR upcalls in legacy provider #17474
Conversation
Move them to their own source file, so they end up in a separate object file. This allows providers to override their implementation to use the corresponding OSSL_FUNC upcalls without having to reimplement everything from crypto/err/err.c.
|
I'm temporarly making this a draft, as I got some other weird errors on my laptop |
|
@rsbeckerca, would you mind trying this out on NonStop? Same configuration that caused the atexit() related crash... |
In progress at commit 5879a0723d. I'll post here with results (2-ish hours). |
There's [ahem] yet another fixup that I pushed just a moment ago. You might want to stop and go with commit 5e46898 instead |
Restarted with that commit. |
Restarted again on e4d879a |
|
Side note: Windows runs currently fail for unrelated reasons |
Thanks for noticing. I think I finally got it in shape to pass all the CI runs |
|
@levitte sorry to say that this fix does not work. SIGSEGV again. |
Ok. Any chance you can get a fall grace for when that happens? |
It isn't very informative. Same as before: |
FYI: This build does a |
|
|
|
@rsbeckerca, I asked for the wrong thing, so let's try again. In your crash case, I assume that |
|
The first call to The second call to These calls are only made once during the execution of the 114 tests. Still |
There appear to be two instances of |
|
Interesting, so there are 2 instances |
|
I found that call stack as well when testing with a That call stack had me realise that the atexit() issue is much more deep seated than I previously understood... running |
I did not monitor for that address in the first run. The critical part here is that data symbols are always duplicated in the architecture OpenSSL uses in this situation. The code probably is too. Essentially, In main: In libcrypto.so: So both symbols are duplicated. |
okay, that is what I would have expected. |
As I have commented before, I think the |
|
It is hard to see what's going on because of the DEFINEs and optimization even at |
Operator error: I built with default |
|
@rsbeckerca, the crashes that you see now, they are in |
This involves the following functions: ERR_new(), ERR_set_debug(), ERR_set_error(), ERR_vset_error(), ERR_set_mark(), ERR_clear_last_mark(), ERR_pop_to_mark(void)
levitte
force-pushed
the
legacy-provider-use-error-upcalls
branch
from
e4d879a
to
b76295e
Compare
|
I've modified this PR to only include the ERR upcall modifications. The atexit() mess is to be handled in a separate PR (I'm working on it) |
test_endecode |
|
9c31ab8 passes both |
|
Final approval anyone? |
|
@rsbeckerca I still don't understand why your shared objects have been unloaded, on your target. |
@bernd-edlinger, that discussion isn't relevant in this PR any more. |
Opened as #17537 |
|
Ping |
paulidale
added
approval: done
|
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. |
levitte
added
approval: ready to merge
Move them to their own source file, so they end up in a separate object file. This allows providers to override their implementation to use the corresponding OSSL_FUNC upcalls without having to reimplement everything from crypto/err/err.c. Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from #17474)
This involves the following functions: ERR_new(), ERR_set_debug(), ERR_set_error(), ERR_vset_error(), ERR_set_mark(), ERR_clear_last_mark(), ERR_pop_to_mark(void) Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from #17474)
This reverts 7ee992a, which linked
test/endecode_teststatically with the legacy provider to avoidhaving
ossl_init_register_atexit()called from inside the provider.We avoid callingossl_init_register_atexit()from inside the providerby reimplementing libcrypto functions that end up doing so, in terms of
upcalls. For the legacy provider, that's the set of ERR upcalls.
Any provider module should use the ERR upcalls for error reporting, or they may be lost (if the provider module is linked with a different libcrypto than it's loaded by).