Hi Nicolas,
Thank you for the patches! I've taken a quick look at the contents, in
hopes of giving you some more feedback for the v2 (in addition to
Mauricio's already suggested changes).
Going through the list, I noticed that there are quite a few patches for a
single bug fix, and that some of them have a substantial diffstat. There's an
impression that these patches are more targeted towards including new features
rather than fixing specific bugs, given their complexity and level of
changes. From the description, I understand most of these changes are probably
required in order to fix faulty behavior, so we should try to get more
information about each patch and what they intend to fix.
Without further context, it's not straightforward to understand why some of the
patches are necessary. As an example, patch 6/7 seems to introduce a new option
related to memory pinning, and patch 7/7 seems to be a Windows-specific change.
It would be very helpful if you could describe your reasoning for each patch in
more detail (e.g. with the "[Other Info]" section of the SRU Bug
Template), something like below:
- patch 0001 is a hard dependency of OPENSSL_INIT_NO_ATEXIT
- patch 0002 implements OPENSSL_INIT_NO_ATEXIT
- patch 0003-0004 add OPENSSL_INIT_NO_ATEXIT to the openssl test suite
...
This would make it easier to understand why each patch included, and help
deciding on whether they would be appropriate for the SRU scenario.
In addition to that, it would be great to have more concrete examples in the
"[Where problems could occur]" section. You mentioned that the patches could
lead to incorrect behavior, so having examples of that would be helpful in
spotting them once the changes go through. For instance, are these problems
expected to show up in openssl itself? Would an external library or program
using openssl start exhibiting different behavior or crashes due to these
changes?
I don't want to come off as excessive, but given that openssl is a
somewhat 'hot' package with potentially big impact on systems, having
good justification and proper context on the patches would be very
beneficial when evaluating the changes.
Thanks again for the help on fixing this bug, and let us know if you
have any questions!
** Changed in: openssl (Ubuntu Bionic)
Status: New => Incomplete
** Changed in: openssl (Ubuntu)
Status: New => Fix Released
** Changed in: openssl (Ubuntu Bionic)
Importance: Undecided => Medium
** Changed in: openssl (Ubuntu Bionic)
Assignee: (unassigned) => Nicolas Bock (nicolasbock)
--
You received this bug notification because you are a member of Ubuntu
Touch seeded packages, which is subscribed to openssl in Ubuntu.
https://bugs.launchpad.net/bugs/1983100
Title:
dotnet build intermittently crashes with segfault on Ubuntu 18.04
Status in openssl package in Ubuntu:
Fix Released
Status in openssl source package in Bionic:
Incomplete
Bug description:
[Impact]
Bionic's OpenSSL 1.1.1 package
(https://launchpad.net/ubuntu/bionic/+source/openssl) is the only
version of openssl 1.1.1 on any distro that we've encountered that
does not have support for the OPENSSL_NO_ATEXIT functionality from
1.1.1b (openssl/openssl@c2b3db2).
The threading model in .NET has the possibility that background
threads are still running when exit() is called, which can cause
SIGSEGV if a background thread interacts with OpenSSL after/while it
has unloaded. For that reason, we always initialize OpenSSL 1.1.1 with
the OPENSSL_NO_ATEXIT flag (which, of all the distros we run on only
has no effect on Bionic).
We feel that the stability of applications on Ubuntu 18.04 would be
improved if the functionality of OPENSSL_NO_ATEXIT was merged into the
bionic openssl 1.1.1 package, even if the constant isn't published
into the header for the dev package.
Context:
https://github.com/dotnet/runtime/issues/48411#issuecomment-1178405101
[Test Plan]
The described behavior can be reproduced by passing the
OPENSSL_NO_ATEXIT to the OPENSSL_init_ssl() call. The application will
terminate with a SEGFAULT. More concretely, a minimal reproducer is:
#include <stdio.h>
#include <openssl/err.h>
#include <openssl/ssl.h>
#ifndef OPENSSL_INIT_NO_ATEXIT
#define OPENSSL_INIT_NO_ATEXIT 0x00080000L
#endif
static void print_error_string()
{
printf("print_error_string:\n");
printf("ERR_reason_error_string(0) => %s\n", ERR_reason_error_string(0));
}
int main(int argc, char* argv[])
{
// register this handler first, so it runs last.
atexit(print_error_string);
OPENSSL_init_ssl(
OPENSSL_INIT_ADD_ALL_CIPHERS |
OPENSSL_INIT_ADD_ALL_DIGESTS |
OPENSSL_INIT_LOAD_CONFIG |
OPENSSL_INIT_NO_ATEXIT |
OPENSSL_INIT_LOAD_CRYPTO_STRINGS |
OPENSSL_INIT_LOAD_SSL_STRINGS,
NULL);
print_error_string();
return 0;
}
Building
$ sudo apt install libssl-dev
$ gcc test.c -lssl -lcrypto
$ ./a.out
print_error_string:
ERR_reason_error_string(0) => (null)
print_error_string:
Segmentation fault (core dumped)
[Where problems could occur]
The patches adds an option to the OPENSSL_init_crypto() function to
disable the default behavior of calling of a cleanup function on
application exit. The patch also includes a few bug fixes around
various initializations that were supposed to happen once when running
threaded but were not.
These changes have the potential for regressions and it is conceivable
that they lead to incorrect behavior. However, I have also backported
and included all new testing functions in the hope that the changed
behavior will get appropriate testing.
To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/openssl/+bug/1983100/+subscriptions
--
Mailing list: https://launchpad.net/~touch-packages
Post to : touch-packages@lists.launchpad.net
Unsubscribe : https://launchpad.net/~touch-packages
More help : https://help.launchpad.net/ListHelp
