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.

Sign up for GitHub

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

Jump to bottom

Properly format IPv6 addresses and return null for unknown addresses #14

Merged
merged 2 commits into from
Jan 23, 2017
Merged

Properly format IPv6 addresses and return null for unknown addresses #14

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e6adf45
Properly format IPv6 addresses and return null for unknown addresses
clue Jan 21, 2017
a6ac236
Accessing socket address must not report errors accross all PHP versions
clue Jan 22, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 9 additions & 14 deletions src/Socket.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,16 +36,12 @@ public function __construct(LoopInterface $loop, $socket, Buffer $buffer = null)

public function getLocalAddress()
{
if ($this->socket !== false) {
return stream_socket_get_name($this->socket, false);
}
return $this->sanitizeAddress(@stream_socket_get_name($this->socket, false));
Copy link
Member

Choose a reason for hiding this comment

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

Is there a specific reason for omitting the $this->socket !== false now?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are several occasions where the stream_socket_get_name() call may emit a warning and unfortunately this is not reliable across different PHP versions and HHVM. Besides, the socket property can accessed from outside of this class so that the false check is simply not reliable either.

I've removed the check completely for consistency with the Socket component and simply discard any error messages from this call.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, my suggestion was not to remove @ from the stream_socket_get_name call again. I've been just curious if there are any consequences to potentially call stream_socket_get_name with false (or anything which isn't a resource).

Copy link
Member Author

Choose a reason for hiding this comment

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

This is all covered by the test suite and we now make sure to either return the correct address string or null if it's unknown. Here's how this works under the hood:

>>> $f = stream_socket_client('google.com:80');
=> stream resource #279
>>> stream_socket_get_name($f, true);
=> "216.58.205.238:80"
>>> fclose($f);
=> true
>>> @stream_socket_get_name($f, true);
PHP warning:  stream_socket_get_name(): supplied resource is not a valid stream resource on line 1
=> false
>>> @stream_socket_get_name(false, true);
PHP warning:  stream_socket_get_name() expects parameter 1 to be resource, boolean given on line 1
=> false

This uses a client socket for simplicity, the server socket emits similar errors but has far more error situations, e.g. the server socket does not have a remote address and returns false without emitting an error (unless you're on HHVM).

Copy link
Member

Choose a reason for hiding this comment

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

Yes, i'm aware that stream_socket_get_namewould return false for non-resources, but we're relying now on PHP triggering a warning.

I also noticed, that i looks like being more inconsistent as before.

Note, that i'm reviewing without too much knowledge about the internals, so my comments are not meant to block a merge.

Copy link
Member Author

Choose a reason for hiding this comment

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

No worries, I actually appreciate these kind of discussions :-)

This PR makes it so that we ignore any warnings (which are unreliable) and simply rely on the function returning false (which is documented behavior and works reliably across different PHP versions and HHVM). This is covered by both our test suite and the above gist.

Once this PR is in, the behavior is consistent throughout the Datagram component.

It is currently not consistent with regards to the Socket component. As you rightfully pointed out, the Socket component does currently not implement any checks and does not suppress any errors either. This means that this does in fact error out when one tries to access to remote address after the socket has been closed. See also reactphp/socket#19, for which I've prepared a PR that I'll file once reactphp/socket#61 is in 👍

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member Author

Choose a reason for hiding this comment

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

For the reference, here's the promised follow-up PR: reactphp/socket#63 👍

}

public function getRemoteAddress()
{
if ($this->socket !== false) {
return stream_socket_get_name($this->socket, true);
}
return $this->sanitizeAddress(@stream_socket_get_name($this->socket, true));
}

public function send($data, $remoteAddress = null)
Expand Down Expand Up @@ -102,16 +98,15 @@ public function end()

private function sanitizeAddress($address)
{
// doc comment suggests IPv6 address is not enclosed in square brackets?
if ($address === false) {
return null;
}

$pos = strrpos($address, ':');
// this is an IPv6 address which includes colons but no square brackets
if ($pos !== false && substr($address, 0, 1) !== '[') {
if (strpos($address, ':') < $pos) {
$port = substr($address, $pos + 1);
$address = '[' . substr($address, 0, $pos) . ']:' . $port;
}

$pos = strrpos($address, ':');
if ($pos !== false && strpos($address, ':') < $pos && substr($address, 0, 1) !== '[') {
$port = substr($address, $pos + 1);
$address = '[' . substr($address, 0, $pos) . ']:' . $port;
}
return $address;
}
Expand Down
23 changes: 23 additions & 0 deletions tests/FactoryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,14 @@ public function testCreateClient()
$capturedClient = Block\await($promise, $this->loop);
$this->assertInstanceOf('React\Datagram\Socket', $capturedClient);

$this->assertEquals('127.0.0.1:12345', $capturedClient->getRemoteAddress());

$this->assertContains('127.0.0.1:', $capturedClient->getLocalAddress());
$this->assertNotEquals('127.0.0.1:12345', $capturedClient->getLocalAddress());

$capturedClient->close();

$this->assertNull($capturedClient->getRemoteAddress());
}

public function testCreateClientLocalhost()
Expand All @@ -35,6 +42,11 @@ public function testCreateClientLocalhost()
$capturedClient = Block\await($promise, $this->loop);
$this->assertInstanceOf('React\Datagram\Socket', $capturedClient);

$this->assertEquals('127.0.0.1:12345', $capturedClient->getRemoteAddress());

$this->assertContains('127.0.0.1:', $capturedClient->getLocalAddress());
$this->assertNotEquals('127.0.0.1:12345', $capturedClient->getLocalAddress());

$capturedClient->close();
}

Expand All @@ -45,6 +57,11 @@ public function testCreateClientIpv6()
$capturedClient = Block\await($promise, $this->loop);
$this->assertInstanceOf('React\Datagram\Socket', $capturedClient);

$this->assertEquals('[::1]:12345', $capturedClient->getRemoteAddress());

$this->assertContains('[::1]:', $capturedClient->getLocalAddress());
$this->assertNotEquals('[::1]:12345', $capturedClient->getLocalAddress());

$capturedClient->close();
}

Expand All @@ -55,7 +72,12 @@ public function testCreateServer()
$capturedServer = Block\await($promise, $this->loop);
$this->assertInstanceOf('React\Datagram\Socket', $capturedServer);

$this->assertEquals('127.0.0.1:12345', $capturedServer->getLocalAddress());
$this->assertNull($capturedServer->getRemoteAddress());

$capturedServer->close();

$this->assertNull($capturedServer->getLocalAddress());
}

public function testCreateServerRandomPort()
Expand All @@ -66,6 +88,7 @@ public function testCreateServerRandomPort()
$this->assertInstanceOf('React\Datagram\Socket', $capturedServer);

$this->assertNotEquals('127.0.0.1:0', $capturedServer->getLocalAddress());
$this->assertNull($capturedServer->getRemoteAddress());

$capturedServer->close();
}
Expand Down