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

Add a safe FFI wrapper in wireguard-go-rs #6299

Conversation

hulthe
Copy link
Contributor

@hulthe hulthe commented May 29, 2024

TODO

  • Resolve TODOs
  • Make sure android works (again)

This change is Reviewable

Copy link

linear bot commented May 29, 2024

@hulthe hulthe self-assigned this May 29, 2024
@hulthe hulthe force-pushed the make-the-rust-bindings-exposed-by-wireguard-go-rs-safe-des-938 branch 2 times, most recently from ab1d16d to 7488bd9 Compare May 29, 2024 14:29
@hulthe hulthe force-pushed the make-the-rust-bindings-exposed-by-wireguard-go-rs-safe-des-938 branch 4 times, most recently from ce54702 to 6d798bd Compare May 29, 2024 15:35
wireguard-go-rs/src/lib.rs Outdated Show resolved Hide resolved
wireguard-go-rs/src/lib.rs Outdated Show resolved Hide resolved
wireguard-go-rs/src/lib.rs Outdated Show resolved Hide resolved
@MarkusPettersson98 MarkusPettersson98 force-pushed the add-daita-cfg-flag branch 4 times, most recently from fc9295f to 346ddac Compare June 4, 2024 08:10
@hulthe hulthe force-pushed the make-the-rust-bindings-exposed-by-wireguard-go-rs-safe-des-938 branch 2 times, most recently from b4fdf36 to 9036306 Compare June 4, 2024 08:23
talpid-wireguard/src/lib.rs Outdated Show resolved Hide resolved
@hulthe hulthe marked this pull request as ready for review June 4, 2024 09:59
Copy link
Contributor

@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.

Reviewed 1 of 10 files at r1, 3 of 80 files at r3, 7 of 13 files at r6, 71 of 76 files at r7, 2 of 2 files at r8, 5 of 5 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @hulthe and @Serock3)


talpid-wireguard/src/connectivity_check.rs line 218 at r1 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

I've checked, and using blocking_lock here is fine. Tokio will panic if this method is called from a tokio runtime

Yeah, I also think it is fine. Maybe add a "Panics"-section to this function's doc comment which echos the same gotcha as blocking_lock?:)


wireguard-go-rs/libwg/libwg.go line 47 at r9 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

Keep or remove? Whaddya think?

Keep if we currently can't replace it with (a) more specific error(s), otherwise remove it in favor of it/those:)


wireguard-go-rs/src/lib.rs line 68 at r10 (raw file):

    /// for the tunnel device and logging. For targets other than android, this also takes an MTU value.
    ///
    /// The `logging_callback` let's you provide a Rust function that receivec any logging output

receives*

Code quote:

receivec

wireguard-go-rs/src/lib.rs line 69 at r10 (raw file):

    ///
    /// The `logging_callback` let's you provide a Rust function that receivec any logging output
    /// from wireguard-go. `logging_context` is an value that will be passed to each invocation of

a*

Code quote:

an

wireguard-go-rs/src/lib.rs line 109 at r10 (raw file):

    ///
    /// **NOTE:** You should take extra care to avoid copying any secrets from the config without zeroizing them afterwards.
    // NOTE: this could return a guard type with a custom Drop impl instead, but me lazy.

Would be neat ;) No need to though

Code quote:

// NOTE: this could return a guard type with a custom Drop impl instead, but me lazy.

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: 83 of 86 files reviewed, 12 unresolved discussions (waiting on @MarkusPettersson98 and @Serock3)


wireguard-go-rs/libwg/libwg.go line 47 at r9 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Keep if we currently can't replace it with (a) more specific error(s), otherwise remove it in favor of it/those:)

We could, but it would involve refactoring wireguard-go. I'm removing this TODO from here as I feel it's slightly out of scope of this PR

Copy link
Contributor

@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.

Reviewed 2 of 2 files at r11, 1 of 1 files at r12, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @hulthe and @Serock3)


talpid-wireguard/src/wireguard_go/mod.rs line 230 at r10 (raw file):

            {
                let socket_v4 = unsafe { wgGetSocketV4(handle) };
                let socket_v6 = unsafe { wgGetSocketV6(handle) };

You did already wrap these in safe functions, right? 😊

Code quote:

                let socket_v4 = unsafe { wgGetSocketV4(handle) };
                let socket_v6 = unsafe { wgGetSocketV6(handle) };

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: 83 of 86 files reviewed, 12 unresolved discussions (waiting on @MarkusPettersson98 and @Serock3)


talpid-wireguard/src/connectivity_check.rs line 218 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Yeah, I also think it is fine. Maybe add a "Panics"-section to this function's doc comment which echos the same gotcha as blocking_lock?:)

Done.


wireguard-go-rs/src/lib.rs line 68 at r10 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

receives*

Done.


wireguard-go-rs/src/lib.rs line 69 at r10 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

a*

Done.

Copy link
Contributor

@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.

Reviewed 3 of 3 files at r13, 1 of 1 files at r14, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @hulthe and @Serock3)

@hulthe hulthe force-pushed the make-the-rust-bindings-exposed-by-wireguard-go-rs-safe-des-938 branch from 3952bac to 30a3e9a Compare June 4, 2024 13:02
@hulthe hulthe requested a review from Serock3 June 4, 2024 13:18
talpid-wireguard/src/wireguard_go/mod.rs Outdated Show resolved Hide resolved
wireguard-go-rs/src/lib.rs Show resolved Hide resolved
wireguard-go-rs/src/lib.rs Show resolved Hide resolved
wireguard-go-rs/libwg/libwg.go Show resolved Hide resolved
talpid-wireguard/src/lib.rs Show resolved Hide resolved
wireguard-go-rs/src/lib.rs Outdated Show resolved Hide resolved
Also:
- Use u64 instead of *mut void as log context
- Make Tunnel::set_config take a &mut self
- Use dyn Error instead of i32s for wg errors
@hulthe hulthe force-pushed the make-the-rust-bindings-exposed-by-wireguard-go-rs-safe-des-938 branch from 30a3e9a to df324b0 Compare June 4, 2024 14:26
Copy link
Contributor

@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.

Reviewed 1 of 2 files at r15, 1 of 1 files at r16, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @hulthe and @Serock3)

@MarkusPettersson98 MarkusPettersson98 merged commit df324b0 into add-daita-cfg-flag Jun 4, 2024
41 of 43 checks passed
@MarkusPettersson98 MarkusPettersson98 deleted the make-the-rust-bindings-exposed-by-wireguard-go-rs-safe-des-938 branch June 4, 2024 14:36
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