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

Made Supernova bind to the specified address, instead of always liste… #5474

Merged
merged 6 commits into from Jun 14, 2021

Conversation

laserbat
Copy link
Contributor

@laserbat laserbat commented Jun 9, 2021

…ning on all interfaces.

Purpose and Motivation

Fixes #4864

Supernova currently binds to 0.0.0.0 regardless of address provided in arguments. This PR makes it honor user supplied address, as well as binding on 127.0.0.1 by default.

As @brianlheim pointed out in #4864, this fix can potentially cause issues if there are multiple supernova servers running on same port but different interfaces. I think it's worth to create a separate issue to discuss how this can be fixed, possibly in a different PR.

Types of changes

  • New feature
  • Breaking change

This PR changes the default (insecure) behavior, by making supernova listen on 127.0.0.1 when no address is supplied. The security improvement, as well as making supernova behave more like scsynth should justify this change.

To-do list

  • Code is tested
  • All tests are passing
  • Updated documentation
  • This PR is ready for review

@laserbat
Copy link
Contributor Author

laserbat commented Jun 9, 2021

Opened an issue to discuss the potential shmem collision: #5475

@laserbat
Copy link
Contributor Author

laserbat commented Jun 9, 2021

Just realized that this fix doesn't work with ipv6 and that I should validate address early, as suggested in @brianlheim's comment. Will correct this ASAP.

The bind address is validated at command line argument parse stage now.
@laserbat
Copy link
Contributor Author

laserbat commented Jun 9, 2021

I fixed ipv6 issue and corrected my code formatting. Hopefully it'll pass the tests

@dyfer dyfer added this to the 3.12.0 milestone Jun 9, 2021
@dyfer
Copy link
Member

dyfer commented Jun 9, 2021

Thank you! This works as advertised, tested on macOS.

I'd like for someone else to also look over this PR before approving/merging.

@laserbat
Copy link
Contributor Author

laserbat commented Jun 9, 2021

Cool! Glad it works on macOS as well. Since you added it to 3.12.0 milestone, should I write that version in the updated documentation? I used "3.12.x" as a placeholder.

@dyfer
Copy link
Member

dyfer commented Jun 9, 2021

@laserbat good point about the docs. Just FYI we weren't planning on getting any new PRs into 3.12, but since it changes the default behavior (and tightens security, and makes supernova behave the same as scsynth) I think it would be desirable to include it.

In that case yes, the docs could be changed stating that the old behavior was in place "Before SuperCollider 3.12" (or 3.12.0). I also think "before" is better than "until" since reading it I wouldn't be sure if the latter means including or excluding that version. But that's a really minor thing, feel free to phrase it the way sounds good to you.

@laserbat
Copy link
Contributor Author

laserbat commented Jun 9, 2021

I see. Thanks for clarifying this, I will update the documentation in a few minutes :)

@joshpar joshpar self-assigned this Jun 13, 2021
Copy link
Member

@joshpar joshpar 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! I think this looks fine to me.

@dyfer dyfer merged commit 45acbf2 into supercollider:develop Jun 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

supernova binds to 0.0.0.0 by default
3 participants