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

Fix mullvad-cli panicking if writing to a closed pipe #7802

Merged
merged 1 commit into from
Mar 12, 2025

Conversation

MarkusPettersson98
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 commented Mar 12, 2025

This PR fixes #7689. I've provided sources to the problem and suggested solution as comments in the code. Please have a look.


This change is Reviewable

@MarkusPettersson98 MarkusPettersson98 requested a review from dlon March 12, 2025 12:46
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 4 of 4 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

dlon
dlon previously approved these changes Mar 12, 2025
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.

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


mullvad-cli/src/main.rs line 159 at r1 (raw file):

    // https://github.com/rust-lang/rust/issues/119980
    // https://github.com/typst/typst/pull/5444
    sigpipe::reset();

I would slightly prefer using libc over another dependency here.

Copy link
Contributor Author

@MarkusPettersson98 MarkusPettersson98 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: 1 of 4 files reviewed, all discussions resolved (waiting on @dlon)


mullvad-cli/src/main.rs line 159 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

I would slightly prefer using libc over another dependency here.

Done WDYT?

@MarkusPettersson98 MarkusPettersson98 force-pushed the fix-broken-pipe-printing-to-stdout branch 4 times, most recently from beabe72 to ec9f3b4 Compare March 12, 2025 14:10
dlon
dlon previously approved these changes Mar 12, 2025
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 3 of 3 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


mullvad-cli/src/main.rs line 159 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Done WDYT?

:lgtm:

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 r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Fix `SIGPIPE` being ignored, which would cause the `mullvad-cli` to
panic if it received a `PIPE` signal (e.g. it was piped into `echo`).
@MarkusPettersson98 MarkusPettersson98 force-pushed the fix-broken-pipe-printing-to-stdout branch from 68fc980 to 54e0bea Compare March 12, 2025 16:11
@MarkusPettersson98 MarkusPettersson98 merged commit 8332181 into main Mar 12, 2025
60 checks passed
@MarkusPettersson98 MarkusPettersson98 deleted the fix-broken-pipe-printing-to-stdout branch March 12, 2025 16:44
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.

mullvad-cli sometimes panics if not printing to stdout?
2 participants