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 crash and connection failure when invalid bind_address is set #274

Merged
merged 3 commits into from
Oct 18, 2024

Conversation

mojyack
Copy link
Contributor

@mojyack mojyack commented Oct 18, 2024

This PR has two patches to fix bugs around udp_create_socket error handling.

The first one is to fix issue where:
If conn_poll_init fails, for example by setting an invalid bind address, and it is the only agent at the time, all newly created agents will fail until another agent is created and deleted.

See this sample code connectivity-bind-address.c.gz.
The former test_connectivity_with_bind_address("8.8.8.8") will fail of course since 8.8.8.8 is an invalid local address(for most computers).
But then the latter test_connectivity_with_bind_address(NULL) will also fail. And if this is executed before 8.8.8.8 case, it will succeed.

This is because the connections thread has already exited, but the registry is not re-created in subsequent conn_create.
The patch fixes this by releasing registry when init_func fails.

The second one is to fix crash when registry_init_func fails when concurrency_mode is set to MUX or THREAD.
You can easily test this by adding config1.concurrency_mode = JUICE_CONCURRENCY_MODE_MUX to attached example.
The patch fixes this by returning explicit error value from acquire_registry and checking it.

Copy link
Owner

@paullouisageneau paullouisageneau left a comment

Choose a reason for hiding this comment

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

Thank you for the fix! I think you should avoid the cast, otherwise it looks good.

src/conn.c Outdated Show resolved Hide resolved
Copy link
Owner

@paullouisageneau paullouisageneau left a comment

Choose a reason for hiding this comment

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

Looking good, thanks!

@paullouisageneau paullouisageneau merged commit 658ec6b into paullouisageneau:master Oct 18, 2024
3 checks passed
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