-
-
Notifications
You must be signed in to change notification settings - Fork 9.9k
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
this change moves stack of compression methods, now global variable #24414
Conversation
6e47851
to
d4a5fb3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I build this I get linker errors:
/usr/bin/ld: ./libssl.so: undefined reference to `ossl_lib_ctx_get_data'
/usr/bin/ld: ./libssl.so: undefined reference to `ossl_find_compression'
/usr/bin/ld: ./libssl.so: undefined reference to `ossl_lib_ctx_set0_compression_methods'
This is because these symbols are not exported from libcrypto. It seems to me the only symbol that you actually need to export from libcrypto is `ossl_lib_ctx_get_data'. The other two could probably just be in libssl.
We have as yet not had the concept of internal symbols that are exported from libcrypto and are privately used by libssl. All symbols exported from libcrypto are public and form part of the API. With that in mind I would actually create a new public libcrypto function OSSL_LIB_CTX_get_data
as a simple wrapper around ossl_lib_ctx_get_data
. The public documentation would say that, although this is a public function, all the indexes that you can pass are reserved for internal use and so applications should not call this function. We might at some point in the future extend it with the idea to support application supplied indexes - but we don't have to worry about that now.
I've not noticed those errors on OpenBSD/clang tool chain. I can see those errors on Debian-12/gcc tool chain. I agree we can remove |
I'm not sure we need to move this to libcrypto. What if the libssl data we stored in the libctx was actually a STACK_OF(SSL_COMP) *SSL_COMP_set0_compression_methods(STACK_OF(SSL_COMP)
*meths)
{
STACK_OF(OSSL_COMP) **data = OSSL_LIB_CTX_get_data(NULL, OSSL_LIB_CTX_COMP_METHODS);
STACK_OF(SSL_COMP) *old_meths = (STACK_OF(SSL_COMP) *)*data;
*data = (STACK_OF(OSSL_COMP) *)meths;
return old_meths;
} |
The updated PR addresses all suggestions from @mattcaswell I failed to figure out how to hook newly added file |
I'll update PR shortly to see how it will look. On the other hand I'm not sure if those changes are heading in desired direction. I feel it goes against the concept 'all data are private' to OSSL_LIB_CTX. Changing |
That isn't actually the current design of OSSL_LIB_CTX. Currently the context.c code mostly has a bunch of internal members which are "black boxes" as far as context.c is concerned. The internals of the black box are private to the module that uses it. E.g. consider |
New version makes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Just a few minor nits remain.
CI failures seem to be due to #24463 |
include/openssl/ssl.h.in
Outdated
typedef struct ssl_comp_st SSL_COMP; | ||
typedef struct ossl_comp_st SSL_COMP; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not change the typedef from ssl_comp_st to ossl_comp_st. Just move it to openssl/comp.h and keep using SSL_COMP everywhere instead of OSSL_COMP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those casts from OSSL_COMP to SSL_COMP are awkward and unnecessary if we just do not introduce the new type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few final tweaks needed
include/openssl/comp.h
Outdated
typedef struct ssl_comp_st { | ||
int id; | ||
const char *name; | ||
COMP_METHOD *method; | ||
} SSL_COMP; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The structure content should be kept internal -> into internal/comp.h
include/openssl/comp.h
Outdated
|
||
STACK_OF(SSL_COMP); | ||
|
||
DEFINE_STACK_OF(SSL_COMP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This must be generated through the generate_stack_macros("SSL_COMP")
perl generator by moving that from ssl.h.in -> i.e., it requires generating comp.h from comp.h.in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering why we are bothering to move it? Why not just keep it in ssl.h.in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to be able to release the stack from libcrypto, so that means the definition needs to be in a header that code linked into libcrypto can include. IMO ssl.h should not be included.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO ssl.h should not be included.
Why not if we're only using it for some typedefs etc? Moving things to comp.h is problematic because it is excluded in the case of a no-comp build - whereas SSL_COMP
should not be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively you just replicate the DEFINE_STACK_OF(SSL_COMP)
line in context.c and not include ssl.h at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the updated PR includes all suggestions from t8m. the code in comp.h.in
got reshuffled so only relevant portion in that file is protected by OPENSSL_NO_COMP guard. I did test and build it:
- perl ./Configure --strict-warnings enable-zlib enable-comp
- perl ./Configure --strict-warnings no-zlib no-brotli no-zstd no-comp
both versions did pass build and make test
include/openssl/ssl.h.in
Outdated
@@ -976,7 +974,6 @@ extern "C" { | |||
*/ | |||
{- | |||
generate_const_stack_macros("SSL_CIPHER") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't this missing ;
?
include/openssl/comp.h.in
Outdated
* | ||
* Licensed under the Apache License 2.0 (the "License"). You may not use | ||
* this file except in compliance with the License. You can obtain a copy | ||
* in the file LICENSE in the source distribution or at | ||
* https://www.openssl.org/source/license.html | ||
*/ | ||
|
||
{- | ||
use OpenSSL::stackhash qw(generate_stack_macros generate_const_stack_macros); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generate_const_stack_macros in't needed
include/openssl/comp.h.in
Outdated
* | ||
* Copyright 4 The OpenSSL Project Authors. All Rights Reserved. | ||
* Copyright (c) 2002, Oracle and/or its affiliates. All rights reserved | ||
* Copyright 2005 Nokia. All rights reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't add this. None of the things moved were IMO added by these copyright holders.
include/openssl/comp.h.in
Outdated
@@ -1,12 +1,20 @@ | |||
/* | |||
* Copyright 2015-2018 The OpenSSL Project Authors. All Rights Reserved. | |||
* Copyright 2024 The OpenSSL Project Authors. All Rights Reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original copyright should be kept 2015-2024
ssl/ssl_ciph.c
Outdated
return ssl_comp_methods; | ||
STACK_OF(SSL_COMP) **rv; | ||
|
||
rv = (STACK_OF(SSL_COMP) **) OSSL_LIB_CTX_get_data(NULL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: no space after **)
in libssl into crypto context. This change is rquired by atexit PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mattcaswell IMO this is OK. Maybe the SSL_COMP stack declarations could have been handled differently but I see this as cleaner way.
typedef struct ssl_comp_st SSL_COMP; | ||
|
||
{- | ||
generate_stack_macros("SSL_COMP"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have preferred to keep this in ssl.h.in. But ok.
This pull request is ready to merge |
Merged to the master branch with reworded commit message. Thank you. |
The compression methods are now a global variable in libssl. This change moves it into OSSL library context. It is necessary to eliminate atexit call from libssl. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #24414)
The compression methods are now a global variable in libssl. This change moves it into OSSL library context. It is necessary to eliminate atexit call from libssl. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#24414)
in libssl into crypto context. This change is rquired by atexit PR.