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.

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

Disable fault_handler logging in release builds (MLLVD-CR-24-02) #7137

Merged
merged 1 commit into from
Nov 7, 2024

Conversation

hulthe
Copy link
Contributor

@hulthe hulthe commented Nov 6, 2024

Also add a reentrancy-guard to the fault handler

Mitigation for MLLVD-CR-24-01 and MLLVD-CR-24-02


This change is Reviewable

@hulthe hulthe requested review from Serock3 and dlon November 6, 2024 13:08
@hulthe hulthe changed the title Disable fault_handler in release builds Disable fault_handler logging in release builds Nov 6, 2024
Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @hulthe)


mullvad-daemon/src/exception_logging/unix.rs line 90 at r1 (raw file):

    // NOTE: `process::abort` is safer than `process::exit` because it doesn't perform any cleanup
    #[cfg(not(debug_assertions))]
    std::process::abort();

Should we instead not register a signal handler at all when debug_assertions is false? I.e. by conditionally compiling INIT_ONCE.call_once(...).

@hulthe hulthe force-pushed the disable-fault-handler-in-release branch 3 times, most recently from 0a3cd46 to 000f570 Compare November 6, 2024 15:41
Copy link
Contributor Author

@hulthe hulthe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @dlon)


mullvad-daemon/src/exception_logging/unix.rs line 90 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Should we instead not register a signal handler at all when debug_assertions is false? I.e. by conditionally compiling INIT_ONCE.call_once(...).

Removed the signal handler completely. Also reworded some comments, lmk what you think.

dlon
dlon previously approved these changes Nov 6, 2024
Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


mullvad-daemon/src/exception_logging/unix.rs line 90 at r1 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

Removed the signal handler completely. Also reworded some comments, lmk what you think.

:lgtm:

Nit: Do you think it would be cleaner to move the code into a module and make only that mod conditional on debug_assertions?

#[cfg(debug_assertions)]
mod inner {
    pub fn enable() {
        static INIT_ONCE: Once = Once::new();
        // ...
    }
}

#[cfg(not(debug_assertions))]
mod inner {
    pub fn enable() {
        // do nothing
    }
}

// TODO: I'm bad at naming?
pub use inner::*;

Serock3
Serock3 previously approved these changes Nov 7, 2024
Copy link
Contributor

@Serock3 Serock3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor Author

@hulthe hulthe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


mullvad-daemon/src/exception_logging/unix.rs line 90 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…
:lgtm:

Nit: Do you think it would be cleaner to move the code into a module and make only that mod conditional on debug_assertions?

#[cfg(debug_assertions)]
mod inner {
    pub fn enable() {
        static INIT_ONCE: Once = Once::new();
        // ...
    }
}

#[cfg(not(debug_assertions))]
mod inner {
    pub fn enable() {
        // do nothing
    }
}

// TODO: I'm bad at naming?
pub use inner::*;

Yes i would

@hulthe hulthe requested review from dlon and Serock3 November 7, 2024 09:17
@hulthe hulthe dismissed stale reviews from Serock3 and dlon via 99b1b31 November 7, 2024 09:18
@hulthe hulthe force-pushed the disable-fault-handler-in-release branch from 000f570 to 99b1b31 Compare November 7, 2024 09:18
Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@hulthe hulthe force-pushed the disable-fault-handler-in-release branch from 99b1b31 to d903147 Compare November 7, 2024 11:41
@hulthe hulthe merged commit f4c609a into main Nov 7, 2024
52 of 53 checks passed
@hulthe hulthe deleted the disable-fault-handler-in-release branch November 7, 2024 12:06
@hulthe hulthe changed the title Disable fault_handler logging in release builds Disable fault_handler logging in release builds (MLLVD-CR-24-02) Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants