Repair build on OpenBSD

[knightjoel]

Description of changes:

Currently, s2n fails to build on OpenBSD. In part this is due to not properly detecting OpenBSD, in part due to differences in behavior between OpenSSL and LibreSSL (which for some time now has been the crypto library which OpenBSD uses), and in part due to typos in s2n code paths which are followed on OpenBSD.

This PR repairs these issues and enables successful build and 'make test' on OpenBSD.

Call-outs:

• Missing from the PR are the CI changes to enable build tests on OpenBSD. I plan to add this and model it after what's already done for FreeBSD.
• s2n_fork_generation_number_test still fails due to an apparent bug in OpenBSD's libc threading code. I am attempting to send a fix for that upstream. (For a test which is known to fail like this, is it appropriate for s2n to skip that test?)

Testing:

• "make test" passes on OpenBSD (save for s2n_fork_generation_number_test without the appropriate fix to OpenBSD libc).
• No regressions running "make test" on FreeBSD (13.1, OpenSSL 1.1.1o)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[harrisonkaiser]

Well this is very concerning. Do you know why we never built this?

[knightjoel]

I do not know why.
It's strange this condition exists but seems to have never been tested and doesn't get built on any of the more popular platforms. Same case with the delta on line 341.

[torben-hansen]

Thanks for this discovery!

First, let's make this clear: s2n-tls uses always-on prediction resistance. There is no escaped issue here even when, in some sub-cases, this piece of code accidentally didn't enter into the build.

Now, for the reason this code was removed during pre-compilation.

Apparently, FreeBSD doesn't define MAP_INHERIT_ZERO, but instead define INHERIT_ZERO. The implementation of the fork-zeroisation feature was done here: https://reviews.freebsd.org/D427. Hence, MAP_INHERIT_ZERO would never be defined on FreeBSD.

To verify this, I spun up a FreeBSD EC2 instance using FreeBSD 13.1. I made the following change to the s2n-tls source code:

diff --git a/utils/s2n_fork_detection.c b/utils/s2n_fork_detection.c
index 546a7a82..5d838657 100644
--- a/utils/s2n_fork_detection.c
+++ b/utils/s2n_fork_detection.c
@@ -31,6 +31,14 @@ typedef __darwin_pthread_once_t pthread_once_t;
 
 #include <sys/mman.h>
 
+#if defined(MAP_INHERIT_ZERO)
+#error "MAP_INHERIT_ZERO defined!"
+#endif
+
+#if defined(INHERIT_ZERO)
+#error "INHERIT_ZERO defined!"
+#endif
+
 /* Not always defined for Darwin */
 #if !defined(MAP_ANONYMOUS)
     #define MAP_ANONYMOUS MAP_ANON

I then ran the build which produced:

root@freebsd:/home/ec2-user/s2n-tls/s2n-build # ninja
[5/486] Building C object CMakeFiles/s2n.dir/utils/s2n_fork_detection.c.o
FAILED: CMakeFiles/s2n.dir/utils/s2n_fork_detection.c.o 
/usr/bin/cc -D_POSIX_C_SOURCE=200809L -I/home/ec2-user/s2n-tls -I/home/ec2-user/s2n-tls/api -pedantic -
[....]
CMakeFiles/s2n.dir/utils/s2n_fork_detection.c.o -c /home/ec2-user/s2n-tls/utils/s2n_fork_detection.c
/home/ec2-user/s2n-tls/utils/s2n_fork_detection.c:39:2: error: "INHERIT_ZERO defined!"
#error "INHERIT_ZERO defined!"
 ^
1 error generated.

A'ha! We need to cater for this FreeBSD-speciality in the code...

Finally, the reason this was not detected in the CI.

There are unit tests that verifies that each fork detection method works in isolation. However, the unit test first have to check whether the fork detection method is supported (otherwise, it doesn't make sense to test the feature, obviously...). But, the function asserting whether the minherit fork detection method is supported uses the existence of MAP_INHERIT_ZERO as a condition. This means that tests that verifies this fork detection method in isolation are skipped.

Note that the CI also runs ALL unit tests on the default configuration. "Configuration" is really a bad choice of words, because you can't change this without source code changes. So, there is no escaped issue.

The following issue #3240 makes the assertion stronger.

[torben-hansen]

For solution: #3674

[harrisonkaiser]

I think code coverage should help us diagnose these kinds of issues. I wonder if there is code build coverage -- to tell you which lines actually get built.

[lrstewart]

@knightjoel, is this ready for review? If not, would you like a maintainer to finish this fix to unblock your OpenBSD build?

[knightjoel]

Hi @lrstewart, this is ready for review save for s2n_fork_generation_number_test which still fails. This test has actually uncovered a deadlock in OpenBSD's libc. I am sending a diff to the project to fix that, but it will take time to see it comitted.

I think we can either a/ leave this in WIP until the fix lands upstream or b/ #ifdef out some/all of s2n_fork_generation_number_test on OpenBSD. Note that even when a fix lands in OpenBSD, it will not be back-ported to the latest release which complicates CI testing for s2n-tls. In that context, option 'b' has some additional value, at least until a release ships with the libc fix incorporated.

[lrstewart]

Could you open an issue for the s2n_fork_generation_number_test + OpenBSD problem you found? I'd like more details before we decide to disable a test. We might also be able to rewrite the test to work around the deadlock, depending on the details.

[goatgoose]

Thanks for the PR!

[knightjoel]

With @goatgoose's suggested changes, all tests still pass.

100% tests passed, 0 tests failed out of 227

Total Test time (real) = 170.46 sec

[goatgoose]

I'm able to build this locally and in a github action, and the tests are passing. However, MEM_PER_CONNECTION needed to be increased for the mem_usage_test to pass in both of these environments:

Suggested change

	#define MEM_PER_CONNECTION 60
	#define MEM_PER_CONNECTION 71

[knightjoel]

Did you use https://github.com/vmactions/openbsd-builder for your GH action? I've been working with that as well (I want to submit a PR here to add OpenBSD to CI) and I noticed mem_usage_teste fails with that action. I haven't found root cause yet.

Could you share uname -a from your local system? Is your local system running virtualized?

[dougch]

Building on a local kvm instance with only 512MB wasn't able to get this test to fail...

openbsd72$ vmstat    
 procs    memory       page                    disks    traps          cpu
 r   s   avm     fre  flt  re  pi  po  fr  sr wd0 cd0  int   sys   cs us sy id
 1  57   37M     92M 2051   0  48  14   0 588 395   0  257  4372  644 22  7 72
openbsd72$ uname -a
OpenBSD openbsd72.my.domain 7.2 GENERIC.MP#758 amd64
openbsd72$ ./build/bin/s2n_mem_test                                                                                                               
Running /home/doug/gitrepos/s2n-tls/tests/unit/s2n_mem_test.c ... PASSED         20 tests

[goatgoose]

I did use openbsd-builder for the action. My local system is virtualized. Here's uname:

openbsd# uname -a
OpenBSD openbsd.amazon.com 7.2 GENERIC#728 amd64

The test passes on both systems with the increase to MEM_PER_CONNECTION.

[knightjoel]

Thank you both for the uname outputs. I'm suspicious--and your data supports this--that there is something different about this test when run under the single-processor OpenBSD kernel vs the multi-proc kernel. The vmactions/openbsd-builder uses the SP kernel and that's also what @goatgoose saw a failure on. @dougch and I are testing on MP systems where the test is successful.

I'm not yet sure how to deal with this.

[goatgoose]

Interesting. It seems to not be related to MP vs SP. I re-installed openBSD in a VM with bsd.mp, and the test still failed for me:

openbsd-mp# uname -a
OpenBSD openbsd-mp.amazon.com 7.2 GENERIC.MP#758 amd64
openbsd-mp# ../../build/bin/s2n_mem_usage_test
Running /home/s2n-tls/tests/unit/s2n_mem_usage_test.c	...
Actual KB per connection: 70
FAILED test 417

I will keep looking into this and try to figure out why there's a difference. I'm using virtualbox as a VM. I wonder if this is somehow related? openbsd-builder also uses this VM.

[knightjoel]

Building on a local kvm instance with only 512MB wasn't able to get this test to fail...

openbsd72$ vmstat    
 procs    memory       page                    disks    traps          cpu
 r   s   avm     fre  flt  re  pi  po  fr  sr wd0 cd0  int   sys   cs us sy id
 1  57   37M     92M 2051   0  48  14   0 588 395   0  257  4372  644 22  7 72
openbsd72$ uname -a
OpenBSD openbsd72.my.domain 7.2 GENERIC.MP#758 amd64
openbsd72$ ./build/bin/s2n_mem_test                                                                                                               
Running /home/doug/gitrepos/s2n-tls/tests/unit/s2n_mem_test.c ... PASSED         20 tests

@dougch, could you look at this again but with s2n_mem_usage_test? Looks like you'll have to be in tests/unit for it to find the PEM files.

~/tmp/s2n-tls/tests/unit% ../../build/bin/s2n_mem_usage_test
Running /home/joel/tmp/s2n-tls/tests/unit/s2n_mem_usage_test.c ... PASSED       1761 tests

[dougch]

Passes with 71 on my vm:

openbsd72$ git log -1 -q|head
commit 5b091111fb8176f6b1e241a3322989d10aae6965
Author: Joel Knight <knight.joel@gmail.com>
Date:   Tue Jan 10 21:02:02 2023 -0700

    bump MEM_PER_CONNECTION to 71 in s2n_mem_usage_test

    Per conversation with @goatgoose and @dougch, this value allows the test
    to pass on single and multi-processor test instances. The test was
    failing on SP instances with MEM_PER_CONNECTION values < 71.
openbsd72$ ../../build/bin/s2n_mem_usage_test
Running /home/doug/gitrepos/s2n-tls/tests/unit/s2n_mem_usage_test.c ... PASSED       1761 tests