Bug 266397 - security/s2n: Various improvements
Summary: security/s2n: Various improvements
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Nuno Teixeira
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-09-13 12:17 UTC by Daniel Engberg
Modified: 2022-09-13 22:38 UTC (History)
1 user (show)

See Also:
eduardo: maintainer-feedback+


Attachments
Patch for s2n (1.29 KB, patch)
2022-09-13 12:17 UTC, Daniel Engberg
no flags Details | Diff
Patch for s2n v2 (1.29 KB, patch)
2022-09-13 12:47 UTC, Daniel Engberg
no flags Details | Diff
full patch (v2) (3.23 KB, patch)
2022-09-13 14:33 UTC, Nuno Teixeira
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Engberg freebsd_committer freebsd_triage 2022-09-13 12:17:28 UTC
Created attachment 236538 [details]
Patch for s2n

- Define LICENSE_FILE
- Use ports framework for unit testing
- Add option for assembly optimization and LTO
- Disable building tests by default
- Disable assembly optimization by default (requires AVX2 and BMI2 support without runtime detection)
- Use CMake helpers provided by framework

Compile and tested on FreeBSD 13.1-STABLE #0 stable/13-n251817-0c4d13c521a (amd64) (make, make test and make plist)
Comment 1 Daniel Engberg freebsd_committer freebsd_triage 2022-09-13 12:17:49 UTC
This port should also be moved (renamed) to match upstream
Comment 2 Daniel Engberg freebsd_committer freebsd_triage 2022-09-13 12:47:13 UTC
Created attachment 236540 [details]
Patch for s2n v2

Fix typo
Comment 3 Nuno Teixeira freebsd_committer freebsd_triage 2022-09-13 14:30:34 UTC
(In reply to Daniel Engberg from comment #0)

- Define LICENSE_FILE
  Isn't "LICENSE" file implicit? I remember someone explaining me long time ago,
  but now I'm not sure.
- Use ports framework for unit testing
  Nice
- Add option for assembly optimization and LTO
  Nice
- Disable building tests by default
  Nice
- Disable assembly optimization by default (requires AVX2 and BMI2 support without runtime detection)
  Nice
- Use CMake helpers provided by framework
  Nice

I will use above example to update other cmake ports, thanks!
Comment 4 Nuno Teixeira freebsd_committer freebsd_triage 2022-09-13 14:33:16 UTC
Created attachment 236541 [details]
full patch (v2)

- removed GH_PROJECT since it matches (new) PORTNAME

Build and testing ...
Comment 5 Nuno Teixeira freebsd_committer freebsd_triage 2022-09-13 14:56:48 UTC
(In reply to Nuno Teixeira from comment #3)
(...)

And ASM option disappears when building on i386 (OPTIONS_DEFINE_amd64)...

Really nice!
Comment 6 Nuno Teixeira freebsd_committer freebsd_triage 2022-09-13 15:06:58 UTC
(In reply to Nuno Teixeira from comment #3)
(...)

And finally I remember test:
---
99% tests passed, 1 tests failed out of 217

Total Test time (real) = 190.15 sec

The following tests FAILED:
         20 - s2n_cipher_suites_test (SEGFAULT)
---
Comment 7 Daniel Engberg freebsd_committer freebsd_triage 2022-09-13 15:10:51 UTC
I'm fairly sure that it was suggested in the end to define LICENSE_FILE whenever possible unfortunately I can't find logs of that discussion right now.

Looks good otherwise :-)
Comment 8 commit-hook freebsd_committer freebsd_triage 2022-09-13 15:17:45 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=79a0481d1c47b13fa8aa7b97803bea264e1fd13f

commit 79a0481d1c47b13fa8aa7b97803bea264e1fd13f
Author:     Daniel Engberg <diizzy@FreeBSD.org>
AuthorDate: 2022-09-13 15:08:48 +0000
Commit:     Nuno Teixeira <eduardo@FreeBSD.org>
CommitDate: 2022-09-13 15:16:48 +0000

    security/s2n-tls: Various improvements

     - Define LICENSE_FILE
     - Use ports framework for unit testing
     - Add option for assembly optimization and LTO
     - Disable building tests by default
     - Disable assembly optimization by default (requires AVX2 and BMI2 support without runtime detection)
     - Use CMake helpers provided by framework
     - Rename s2n -> s2n-tls to match upstream name

    PR:             266397

 MOVED                                            |  1 +
 security/Makefile                                |  2 +-
 security/{s2n => s2n-tls}/Makefile               | 24 +++++++++++++++---------
 security/{s2n => s2n-tls}/distinfo               |  0
 security/{s2n => s2n-tls}/files/patch-bin_s2nd.c |  0
 security/{s2n => s2n-tls}/files/pkg-message.in   |  0
 security/{s2n => s2n-tls}/pkg-descr              |  0
 security/{s2n => s2n-tls}/pkg-plist              |  0
 8 files changed, 17 insertions(+), 10 deletions(-)
Comment 9 Daniel Engberg freebsd_committer freebsd_triage 2022-09-13 15:29:41 UTC
(In reply to Nuno Teixeira from comment #6)
It doesn't fail using GCC on my so I would try to get a debug builid and possibly enable verbose output for cmake to see what's going on. You might also want to poke upstream about it.
Comment 10 Nuno Teixeira freebsd_committer freebsd_triage 2022-09-13 15:35:25 UTC
(In reply to Daniel Engberg from comment #7)

I've searched for LICENSE_FILE issue too, but didn't find what I was looking for.
I will define it on next commits.
Comment 11 Nuno Teixeira freebsd_committer freebsd_triage 2022-09-13 15:37:31 UTC
(In reply to Daniel Engberg from comment #9)

I will try to get a good log about this fail test and open an upstream issue and try to manage on how to use gcc on this port.
Comment 12 Nuno Teixeira freebsd_committer freebsd_triage 2022-09-13 15:39:06 UTC
I've bookmarked this PR :)

Committed, thank you!
Comment 13 Daniel Engberg freebsd_committer freebsd_triage 2022-09-13 15:42:20 UTC
Sounds great!

Just add USE_GCC= yes to the port to use default GCC version but its less than ideal for a permanent solution
Comment 14 Nuno Teixeira freebsd_committer freebsd_triage 2022-09-13 16:24:26 UTC
(In reply to Daniel Engberg from comment #13)

Ok, I will use it just for test
Comment 15 Daniel Engberg freebsd_committer freebsd_triage 2022-09-13 18:52:09 UTC
No, that just masks the issue which still leaves you with something potentially broken. Either use GCC for everything or (preferably report upstream), please verify yourself that using GCC works before reporting it upstream.
Comment 16 Nuno Teixeira freebsd_committer freebsd_triage 2022-09-13 19:17:55 UTC
(In reply to Daniel Engberg from comment #15)

Yes, that's the plan that I have in mind.
I will make all tests and get a good log and use it for upstream issue (clang).

GCC will be used just to compare results and have an idea if it works or not, as personal extra info.
The important point here is that upstream fixes it with clang.

I understand GCC use just in last place like for example:

---
# clang is crashing on CURRENT due an assertion
# see https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=234232
# and https://bugs.llvm.org/show_bug.cgi?id=40985
.if ${OSVERSION} >= 1400000
USE_GCC=        yes
.endif
---
on devel/aws-checksums but this is a special case that last time I tested with clang it continues to fail and upstream issues are still open.
Comment 17 Nuno Teixeira freebsd_committer freebsd_triage 2022-09-13 22:38:33 UTC
https://github.com/aws/s2n-tls/issues/3489

I'm using manual cmake to make it easier to change flags, etc:
---
cmake -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=/usr \ -DBUILD_SHARED_LIBS=ON -DUNSAFE_TREAT_WARNINGS_AS_ERRORS=OFF -S . -B build
cmake --build build
cmake --build build --target test
Running tests..

99% tests passed, 1 tests failed out of 217

Use "--rerun-failed --output-on-failure" to re-run the failed cases verbosely.
---
Don't know yet how to use ctest "--rerun-failed --output-on-failure"...

Letś see if upstream ask me for more verbose commands.

Cheers