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

Don't block the default route listener while calling REGISTER_IP_ADDRESSES #4218

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

dlon
Copy link
Member

@dlon dlon commented Dec 14, 2022

As above. The ioctl call can take some time to complete (mostly due to the transaction lock being blocked). If that occurs, the offline monitor will be affected as well. This PR simply queues up the calls on a different thread.

Another change that this PR makes is to simply log errors instead of forcing the tunnel state machine into the error state. There's no need to do so: failing to update the IPs cannot cause any leaks into the tunnel since the tunnel IP is static. From a security standpoint, moving to the error state is unnecessary.

Finally, also log any timeout that occurs on the split tunnel "request handling" thread.


This change is Reviewable

Copy link
Contributor

@Jontified Jontified left a comment

Choose a reason for hiding this comment

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

Like we talked about, it might be better to have the daemon block if there is a problem with the split tunneling rather than it silently failing. The ideal would be some way to notify users about non-security critical problems without blocking the tunnel but since that would require UI changes for now blocking seems preferable.

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


talpid-core/src/split_tunnel/windows/mod.rs line 891 at r1 (raw file):

                        )
                    );
                    maybe_send(TunnelCommand::Block(ErrorStateCause::SplitTunnelError));

Like we discussed keeping this is probably a good idea

@dlon dlon force-pushed the win-st-unblock-ip-update branch 2 times, most recently from fbe6abb to 08d3e10 Compare December 22, 2022 14:05
Copy link
Member Author

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


talpid-core/src/split_tunnel/windows/mod.rs line 891 at r1 (raw file):

Previously, Jontified wrote…

Like we discussed keeping this is probably a good idea

Done.

Jontified
Jontified previously approved these changes Jan 9, 2023
Copy link
Contributor

@Jontified Jontified 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 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@dlon dlon force-pushed the win-st-unblock-ip-update branch 2 times, most recently from 0cf2698 to 70c409d Compare September 2, 2024 15:50
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.

2 participants