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

Use socket instead of ping command when pinging on android #7728

Merged
merged 1 commit into from
Feb 27, 2025

Conversation

Pururun
Copy link
Contributor

@Pururun Pururun commented Feb 26, 2025


This change is Reviewable

@Pururun Pururun added the Android Issues related to Android label Feb 26, 2025
@Pururun Pururun requested review from Rawa, dlon and hulthe February 26, 2025 17:12
Copy link

linear bot commented Feb 26, 2025

@Pururun Pururun requested a review from kl February 26, 2025 17:14
Rawa
Rawa previously approved these changes Feb 26, 2025
@Rawa
Copy link
Contributor

Rawa commented Feb 26, 2025

We should add some message, explaining why this fixes droid-1825

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 2 of 4 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions


talpid-wireguard/src/connectivity/pinger/mod.rs line 1 at r2 (raw file):

#[path = "icmp.rs"]

I think we should remove the renaming now:

mod icmp;

talpid-wireguard/Cargo.toml line 40 at r2 (raw file):

[target.'cfg(target_os="android")'.dependencies]
duct = "0.13"

duct is unused, I believe. (Remove these two lines.)


talpid-wireguard/src/connectivity/pinger/icmp.rs line 73 at r2 (raw file):

                Type::DGRAM
            } else {
                Type::DGRAM

Should be Type::RAW

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: all files reviewed, 4 unresolved discussions (waiting on @Pururun)


talpid-wireguard/src/connectivity/pinger/icmp.rs line 65 at r2 (raw file):

    pub fn new(
        addr: Ipv4Addr,
        #[cfg(not(any(target_os = "windows", target_os = "android")))] interface_name: String,

Nit: #[cfg(any(target_os = "linux", target_os = "macos"))]

Copy link
Contributor Author

@Pururun Pururun 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: 2 of 5 files reviewed, 1 unresolved discussion (waiting on @dlon)


talpid-wireguard/Cargo.toml line 40 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

duct is unused, I believe. (Remove these two lines.)

Done


talpid-wireguard/src/connectivity/pinger/icmp.rs line 65 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

Nit: #[cfg(any(target_os = "linux", target_os = "macos"))]

Done


talpid-wireguard/src/connectivity/pinger/icmp.rs line 73 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

Should be Type::RAW

Done


talpid-wireguard/src/connectivity/pinger/mod.rs line 1 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

I think we should remove the renaming now:

mod icmp;

Done

@Pururun Pururun force-pushed the investigate-go-crash-issue-with-runtimegc-droid-1825 branch from 54d95fa to 42a13c4 Compare February 27, 2025 08:35
Copy link
Contributor

@Rawa Rawa 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 5 files reviewed, 2 unresolved discussions (waiting on @dlon)


-- commits line 2 at r4:
Please add some comment with why it relates to DROID-1825 and what it fixes so if we ever hit this again we won't have to spend an eternity. 🙏

Copy link
Contributor

@Rawa Rawa 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 4 files at r1, 2 of 3 files at r3, 1 of 1 files at r4.
Reviewable status: 4 of 5 files reviewed, 2 unresolved discussions (waiting on @dlon and @Pururun)

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 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Pururun)

@Pururun Pururun force-pushed the investigate-go-crash-issue-with-runtimegc-droid-1825 branch from 42a13c4 to 6900a18 Compare February 27, 2025 08:56
Copy link
Contributor Author

@Pururun Pururun 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: all files reviewed, 1 unresolved discussion (waiting on @Rawa)


-- commits line 2 at r4:

Previously, Rawa (David Göransson) wrote…

Please add some comment with why it relates to DROID-1825 and what it fixes so if we ever hit this again we won't have to spend an eternity. 🙏

Updated

Copy link
Contributor

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

Previous implementation spawned a process with tokio which in turn
registered a signal handler without ONASTACK flag set.
When using GO code, all signal handlers needs to have this flag
set otherwise a signal might be handled on a goroutine thread
which has a small stack and thus can overflow.

Reference: DROID-1825

Co-authored-by: David Lönnhager <[email protected]>
@Pururun Pururun force-pushed the investigate-go-crash-issue-with-runtimegc-droid-1825 branch from 6900a18 to 56cde3c Compare February 27, 2025 10:55
@Pururun Pururun merged commit cfbf407 into main Feb 27, 2025
49 of 50 checks passed
@Pururun Pururun deleted the investigate-go-crash-issue-with-runtimegc-droid-1825 branch February 27, 2025 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android Issues related to Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants