ext/sockets: follow-up on AF_PACKET support. #17657
Conversation
devnexen
force-pushed
the
socket_ether_linux2
branch
6 times, most recently
from
ab4ee3d to
d4a18c7
Compare
devnexen
force-pushed
the
socket_ether_linux2
branch
3 times, most recently
from
08f7cff to
fb25000
Compare
devnexen
force-pushed
the
socket_ether_linux2
branch
from
fb25000 to
d6ac80e
Compare
|
cc @arnaud-lb as you reviewed part 1 |
| final class SocketEthernetInfo | ||
| { | ||
| /** @readonly **/ | ||
| public Socket $socket; |
There was a problem hiding this comment.
Suggestion: Add a constructor taking a raw packet as argument, so that people can create their own instances of this class. This would be useful at least in tests.
There was a problem hiding this comment.
Yes I planned this at some point.
|
|
||
| if (UNEXPECTED(!if_indextoname(sll.sll_ifindex, ifrname))) { | ||
| PHP_SOCKET_ERROR(php_sock, "unable to get the interface name", errno); | ||
| zend_string_efree(recv_buf); | ||
| RETURN_FALSE; | ||
| } | ||
|
|
||
| ZEND_TRY_ASSIGN_REF_NEW_STR(arg2, recv_buf); | ||
| struct ethhdr *e = (struct ethhdr *)ZSTR_VAL(recv_buf); |
There was a problem hiding this comment.
My understanding is that this is only valid for SOCK_RAW packets (not SOCK_DGRAM). As only SOCK_RAW is supported yet, we should error when an other socket type is passed, until we add support for them.
| @@ -2156,3 +2161,19 @@ function socket_wsaprotocol_info_import(string $info_id): Socket|false {} | |||
|
|
|||
| function socket_wsaprotocol_info_release(string $info_id): bool {} | |||
| #endif | |||
|
|
|||
| #ifdef AF_PACKET | |||
| final class SocketEthernetInfo | |||
There was a problem hiding this comment.
Suggestion: In order to reduce the overhead of converting the raw packet to an object, we could make all the props virtual and store the raw packet directly in zend_object (in place of properties_table). Additionally we could expose the raw packet as a property. We could even reuse SocketEthernetInfo instances if we let users pass an instance as argument.
There was a problem hiding this comment.
I ll see what I can do in a following PR
| /** @readonly **/ | ||
| public array $payload; |
There was a problem hiding this comment.
Suggestion: Expose the payload as a raw string, and expose the parsed payload separately as an object (null when we don't support the protocol). The object would be instantiated on the fly, too.
There was a problem hiding this comment.
Ok as object, not sure of the value of raw payload but I can always add it sure.
There was a problem hiding this comment.
The raw payload would be useful when the application wants to log packets, or forward them without changing. It's also useful when the object is null due to not being a supported protocol. Also, having the raw payload (always) and the object (when supported) is forward compatible.
There was a problem hiding this comment.
Oh I see. I can get behind it indeed then.
support from socket_recvfrom and exposing basic PHP userland type.
does not make sense, we just bind to raw packets.
devnexen
force-pushed
the
socket_ether_linux2
branch
from
d6ac80e to
d59fdaa
Compare
devnexen
force-pushed
the
socket_ether_linux2
branch
from
d59fdaa to
da14f3e
Compare
|
There's a nightly failure on master+alpine caused by this, pls take a look. |
| #ifdef AF_PACKET | ||
| final class SocketEthernetInfo | ||
| { | ||
| /** @readonly **/ |
There was a problem hiding this comment.
Why did you use the @readonly annotation instead of real readonly properties?
There was a problem hiding this comment.
This object will be rewritten in a following PR anyway.
|
ok looking |
|
cannot reproduce locally the UAF, will revert to unblock the situation. |
support from socket_recvfrom and exposing basic PHP userland type.