Socket ether linux step2

[devnexen]

No description provided.

[bukka]

This has a bit of class design so it should be probably at least posted to internals (it's not like adding a constant or a simple funciton). I agree that there should be some namespacing as well possibly. Definitely not the current names that will easily result in BC break for appliations.

[devnexen]

ping @arnaud-lb I think I have, at least, addressed the majority of your remarks :).

[arnaud-lb]

Sorry for the delay, I missed you ping

[arnaud-lb]

I found no more issues apart from a few comments.

This feels safe as all accesses to the read buffer are guarded by php_socket_get_chunk(). However, we often use this function only for checking the length, and we copy the buffer again after calling it. Maybe the function could be changed to something like this, instead:

typedef struct _php_socket_chunk {
    const char *c;
    size_t chunk_len; /* length of this chunk */
    size_t buf_len; /* length of this chunk plus everything after the chunk in c */
} php_socket_chunk;

static zend_result php_socket_get_chunk(php_socket_chunk *chunk, const php_socket_chunk *src, size_t offset, size_t len) {
	if (UNEXPECTED((offset > SIZE_MAX - len) || offset + len > src->buf_len)) {
		return FAILURE;
	}

        dest->c = src->c + offset;
        dest->chunk_len = len;
        dest->buf_len = src->buf_len - offset;
	return SUCCESS;
}

php_socket_chunk *chunk = {
    .s = ZSTR_VAL(recv_buf),
    .buf_len = ZSTR_LEN(recv_buf),
};

if (php_socket_get_chunk(chunk, chunk, 0, ETH_HLEN) == FAILURE) {
    ...
}

struct ethhdr e;
memcpy(&e, ethhdr_chunk->c, ETH_HLEN);

if (php_socket_get_chunk(chunk, chunk, 0, sizeof(struct iphdr)) == FAILURE) {
    ...
}

struct iphdr ip;
memcpy(&ip, ethhdr_chunk->c, sizeof(ip));

[arnaud-lb]

This could be replaced by a ZEND_ASSERT(), as both slen and ETH_HLEN are compile time constants.

[arnaud-lb]

With recv_flags & MSG_TRUNC, it is possible that retval is longer than ZSTR_LEN(recv_buf).

I suggest that we allow only a finite set of known and supported flags in recv_flags.

[arnaud-lb]

Also, ZSTR_LEN(recv_buf) may be larger than retval. You need to update ZSTR_LEN(recv_buf) after this call, otherwise you expose uninitialized memory past the packet data:

$s_c     = socket_create(AF_PACKET, SOCK_RAW, ETH_P_ALL);
$s_bind  = socket_bind($s_c, 'lo');
$s_s     = socket_create(AF_PACKET, SOCK_RAW, ETH_P_LOOP);
$v_bind  = socket_bind($s_s, 'lo');

$buf = pack("H12H12n", "ffffffffffff", "000000000000", ETH_P_LOOP);
$buf .= str_repeat("A", 46);

printf("buf len: %d\n", strlen($buf));

socket_sendto($s_s, $buf, strlen($buf), 0, "lo", 1);
socket_recvfrom($s_c, $rsp, strlen($buf)+1000, 0, $addr);

var_dump(bin2hex($rsp->rawPacket));

[arnaud-lb]

This macro is unused

[arnaud-lb]

I don't understand this check. Is it necessary, when the length of ipdata is already checked by php_socket_get_chunk()? Same remark for php_socket_afpacket_add_udp().

[arnaud-lb]

This allows to pass any flag as long as recv_flags contains one of MSG_PEEK|MSG_DONTWAIT|MSG_ERRQUEUE. You probably want something like if (recv_flags & ~(MSG_PEEK|MSG_DONTWAIT|MSG_ERRQUEUE)) { error }.

[arnaud-lb]

I'm not sure, but I think you wanted to set offset to ETH_HLEN here.

[devnexen]

yes that explain and the code typo issue below some tests inconsistencies

[arnaud-lb]

Since most of these fields have a fixed/deterministic value, I suggest to check their exact value (without % placeholders). Including the lengths (e.g. string(%d) -> string(42)).

For binary fields such as rawPacket and payload, keep using a placeholder, but also dump them separately in hex.

Same remark for the entire test and other tests.