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

Return the actual bound address, not a hardcoded "localhost". #68

Merged
merged 5 commits into from
Jun 30, 2015

Conversation

bemasc
Copy link
Contributor

@bemasc bemasc commented Jun 29, 2015

This address will always be either '127.0.0.1' or '0.0.0.0',
because nsIUDPSocket::init only supports binding to localhost
or all-interfaces.

Addresses #62 and #67

@trevj

this._nsIUDPSocket.init(port, false);
var isLocal = address === '127.0.0.1' ||
address === 'localhost' ||
address.match(/^(0*:)+0*1$/);

Choose a reason for hiding this comment

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

Why the 1 at the end of this regex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I added a comment. Does it help?

@agallant
Copy link

👍

@bemasc
Copy link
Contributor Author

bemasc commented Jun 29, 2015

I had a conversation with Trevor and changed the logic here to be much more restrictive. Instead of binding to a different address than the one the caller requested, it now returns an error code.

However, this will actually break uProxy ... so please don't press "Merge" so we have time to update uProxy first!

@bemasc
Copy link
Contributor Author

bemasc commented Jun 29, 2015

uProxy change here: UWNetworksLab/uProxy-lib#212

@@ -23,7 +38,7 @@ UDP_Firefox.prototype.bind = function(address, port, continuation) {

UDP_Firefox.prototype.getInfo = function(continuation) {
var returnValue = {
localAddress: "localhost",
localAddress: this._nsIUDPSocket.localAddr.address,
localPort: this._nsIUDPSocket.port

Choose a reason for hiding this comment

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

It may be good to pull port from this._nsIUDPSocket.localAddr.port for consistency - from logging the tests it looks to have the same value as this._nsIUDPSocket.port, so it should work the same, but look more consistent and maybe inoculate us from weird edge cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@agallant
Copy link

Consider the port suggestion, but aside from that 👍

bemasc added a commit that referenced this pull request Jun 30, 2015
Return the actual bound address, not a hardcoded "localhost".
@bemasc bemasc merged commit 1990bbb into master Jun 30, 2015
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