Skip to content

main/network: Use more appropriate types #15511

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

Merged
merged 2 commits into from
Aug 22, 2024

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Aug 21, 2024

And check directly against 0 for success for functions not returning a zend_result.

While going through the file, I noticed a few things:

Signed socklen_t on Windows

I don't really understand why socklen_t is a signed integer on Windows rather than unsigned.

php-src/main/php.h

Lines 192 to 198 in 69d9c12

#ifndef HAVE_SOCKLEN_T
# ifdef PHP_WIN32
typedef int socklen_t;
# else
typedef unsigned int socklen_t;
# endif
#endif

poll(2) emulation

We define our own poll function when PHP_USE_POLL_2_EMULATION is set, but I can't seem to find when it is set, is this related to Windows @cmb69 ?

Usage of gethostbyname_r()

It seems like this function is obsolete and

Applications should use getaddrinfo(3), getnameinfo(3), and gai_strerror(3) instead.

according to https://man7.org/linux/man-pages/man3/gethostbyname.3.html

So maybe we should review this code to use more modern functions?

It seems getting rid of them would also allow us to get rid of:

php-src/ext/standard/file.h

Lines 106 to 110 in 69d9c12

#ifdef HAVE_GETHOSTBYNAME_R
struct hostent tmp_host_info;
char *tmp_host_buf;
size_t tmp_host_buf_len;
#endif

@cmb69
Copy link
Member

cmb69 commented Aug 21, 2024

I don't really understand why socklen_t is a signed integer on Windows rather than unsigned.

Because WinSock does not define socklen_t, but rather expects ints, e.g. https://learn.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-recvfrom (WinSock has been ported from an old BSD implementation, to my knowledge). That is unfortunate, because having to deal with either unsigned or signed makes it harder to reason about the code, and is likely overlooked from those with a POSIX background.

We define our own poll function when PHP_USE_POLL_2_EMULATION is set, but I can't seem to find when it is set, is this related to Windows @cmb69 ?

php-src/main/php_network.h

Lines 124 to 158 in 0775b99

#if defined(HAVE_POLL)
# if defined(HAVE_POLL_H)
# include <poll.h>
# elif defined(HAVE_SYS_POLL_H)
# include <sys/poll.h>
# endif
typedef struct pollfd php_pollfd;
#else
typedef struct _php_pollfd {
php_socket_t fd;
short events;
short revents;
} php_pollfd;
PHPAPI int php_poll2(php_pollfd *ufds, unsigned int nfds, int timeout);
#ifndef POLLIN
# define POLLIN 0x0001 /* There is data to read */
# define POLLPRI 0x0002 /* There is urgent data to read */
# define POLLOUT 0x0004 /* Writing now will not block */
# define POLLERR 0x0008 /* Error condition */
# define POLLHUP 0x0010 /* Hung up */
# define POLLNVAL 0x0020 /* Invalid request: fd not open */
#endif
# ifndef PHP_USE_POLL_2_EMULATION
# define PHP_USE_POLL_2_EMULATION 1
# endif
#endif
#define PHP_POLLREADABLE (POLLIN|POLLERR|POLLHUP)
#ifndef PHP_USE_POLL_2_EMULATION
# define php_poll2(ufds, nfds, timeout) poll(ufds, nfds, timeout)
#endif

So we use our own stuff, whenever poll() is not available. That might be relevant for Windows only (in which case we could simplify), but maybe poll() is not available on some other systems.

So maybe we should review this code to use more modern functions?

Likely a good idea. Note though that our network implementation is a danger zone.

@devnexen may have more insights about this.

@Girgias Girgias marked this pull request as ready for review August 21, 2024 11:43
@Girgias Girgias requested a review from bukka as a code owner August 21, 2024 11:43
@devnexen
Copy link
Member

So we use our own stuff, whenever poll() is not available. That might be relevant for Windows only (in which case we could simplify), but maybe poll() is not available on some other systems.

I agree with this statement, every unix supported by php implements poll, including Haiku (I double checked, it is implemented fully kernel wise and all).

And check directly against 0 for success for functions not returning a zend_result
Not sure why it even is here
@Girgias Girgias force-pushed the network-main-zend_result branch from c41d1ea to 8a9c845 Compare August 21, 2024 22:00
@Girgias
Copy link
Member Author

Girgias commented Aug 21, 2024

I split the two discussion points into their own issues so that it is not forgotten. :)

@Girgias Girgias merged commit 35fbb00 into php:master Aug 22, 2024
10 checks passed
@Girgias Girgias deleted the network-main-zend_result branch August 22, 2024 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants