Alextopher / simple-auth

0 stars 0 forks source link

audit all `unwrap`s and `expect`s from the NSS module, catch_panic them into NssStatus::Unavail #2

Open Alextopher opened 9 months ago

Alextopher commented 9 months ago

I found in my earlier testing experience that cases where the NSS module hung or failed to load would cause system hangs.

Since this is rust we could probably guarantee freedom from crashes by exhaustively removing unwraps.

libnss gives the falling options for Reponses

#[allow(dead_code)]
#[derive(Debug, Clone, Copy, PartialEq)]
pub enum NssStatus {
    TryAgain = -2,
    Unavail = -1,
    NotFound = 0,
    Success = 1,
    Return = 2,
}

Is NssStatus::Unavail better than unwraps?

emberian commented 9 months ago

The unwraps are primarily for mutexes and the expects are wayyy better than just Unavail: they include a message about the cause!

If we do want to do this, the right thing is to use catch_panic at the FFI boundary instead of relying on the rust runtime to turn it into a process abort for us. See also nomicon.

emberian commented 9 months ago

If you find any unwraps that are unjustified, please replace it with an expect. I expect unwraps to be logic errors or unrecoverable initialization failures.

Alextopher commented 9 months ago

When I was working on this project last my system would hang if something bad happened in the NSS module.

Logging to a file or to stderr should be fine.