Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Crash in [@ core::result::unwrap_failed | neqo_crypto::init::closure$0] #1675

Closed
KershawChang opened this issue Feb 23, 2024 · 2 comments · Fixed by #1775 or #2030
Closed

Crash in [@ core::result::unwrap_failed | neqo_crypto::init::closure$0] #1675

KershawChang opened this issue Feb 23, 2024 · 2 comments · Fixed by #1775 or #2030

Comments

@KershawChang
Copy link
Collaborator

See https://bugzilla.mozilla.org/show_bug.cgi?id=1854653

Not sure why we can't init NSS, but maybe we should not crash.
@larseggert
Copy link
Collaborator

larseggert commented Feb 26, 2024 •
edited
Loading

Looking at the code, we use secstatus_to_res(...).expect(...) in a bunch of places in neqo_crypto/src/lib.rs, not only in init(). I guess we should return an error (and not crash) for all of these?

CC @martinthomson

larseggert added a commit to larseggert/neqo that referenced this issue Mar 26, 2024
@larseggert
fix: Don't panic in neqo_crypto::init()
79a8ac7 
Return `Res<()>` instead.

Fixes mozilla#1675
@larseggert larseggert mentioned this issue Mar 26, 2024
Merged
github-merge-queue bot pushed a commit that referenced this issue Mar 28, 2024
@larseggert @martinthomson
fix: Don't panic in neqo_crypto::init() (#1775)
3151adc 
* fix: Don't panic in `neqo_crypto::init()`

Return `Res<()>` instead.

Fixes #1675

* Update neqo-bin/src/bin/client/main.rs

Co-authored-by: Martin Thomson <mt@lowentropy.net>
Signed-off-by: Lars Eggert <lars@eggert.org>

* Update neqo-crypto/src/lib.rs

Co-authored-by: Martin Thomson <mt@lowentropy.net>
Signed-off-by: Lars Eggert <lars@eggert.org>

* Update neqo-crypto/tests/init.rs

Co-authored-by: Martin Thomson <mt@lowentropy.net>
Signed-off-by: Lars Eggert <lars@eggert.org>

* Address code review

* Try and improve coverage

* Fix the `nss_nodb` case

* Fail tests if `init()` fails

* Address code review

* Update neqo-bin/src/bin/client/main.rs

Co-authored-by: Martin Thomson <mt@lowentropy.net>
Signed-off-by: Lars Eggert <lars@eggert.org>

* InternalError -> CryptoError

* Fix merge

* clippy

---------

Signed-off-by: Lars Eggert <lars@eggert.org>
Co-authored-by: Martin Thomson <mt@lowentropy.net>
@larseggert
Copy link
Collaborator

Reopening this, since we still see related crashes.

@larseggert larseggert reopened this Aug 2, 2024
larseggert added a commit to larseggert/neqo that referenced this issue Aug 2, 2024
@larseggert
fix: Don't assert in NSS version_check
03fe5e6 
Instead, pass an error up.

Fixes mozilla#1675 (again, hopefully)
@larseggert larseggert mentioned this issue Aug 2, 2024
Merged
larseggert added a commit that referenced this issue Aug 2, 2024
@larseggert
fix: Don't assert in NSS version_check (#2030)
0eb9174 
Instead, pass an error up.

Fixes #1675 (again, hopefully)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@larseggert @KershawChang