ext/sockets: Additional refactorings to socket_set_option()
#17186
Conversation
…not passing string length
And replace calls to strlcpy to memcpy
|
Don't think we have a CI set-up where |
|
gonna try your branch |
| --SKIPIF-- | ||
| <?php | ||
|
|
||
| if (!defined('IPPROTO_TCP')) { |
There was a problem hiding this comment.
IPPROTO_TCP had been translated into SOL_TCP, also no need to check its existence it s unconditionally set looking at the stub.
It is supported since FreeBSD 13.x, should be no problem. |
ext/sockets/sockets.c
Outdated
| } | ||
| struct accept_filter_arg af = {0}; | ||
| strlcpy(af.af_name, Z_STRVAL_P(arg4), sizeof(af.af_name)); | ||
| if (Z_STRLEN_P(arg4) > sizeof(af.af_name)) { |
There was a problem hiding this comment.
Should that be Z_STRLEN_P(arg4) >= sizeof(af.af_name) (or Z_STRLEN_P(arg4)+1 > sizeof(af.af_name)) to account for the null byte?
There was a problem hiding this comment.
Like here, it would be nice to have more explanations of why you feel we need to replace strlcpy with memcpy.
1/ It is easier to fall into overflows with memcpy as per Arnaud/my comments.
2/ If you want to check truncations, strlcpy return value can help in this.
3/ If this is performance gain and since those changes are mainly about FreeBSD parts, let's say we create a bunch of sockets and we set bbr tcp stack for each so how much of a difference it makes in practice (related buffers are respectively 32 and 16 larges) ? Albeit freebsd's memcpy finally caught up a lot in term of optimisations (highlighting the case for size <= 32 bytes here) since the 14.x release serie. In comparison our strlcpy version is very straightforward but today's compilers are smarter.
Whether the changes are justified, I do not think it is very urgent we can see after Xmas days no rush :)
There was a problem hiding this comment.
Because strlcpy() is bad as I just replied to that other thread, we also need to do extra work to instrumentalize this on top of using a non standard C function.
I don't mind adding a helper function if that's the concern.
There was a problem hiding this comment.
I see ... in this case you still need to address arnaud and my comment.
There was a problem hiding this comment.
Yeah, I'll get round to it after the Xmas break :)
No description provided.