From becf2e3feb7d5c8998694b3f281c9ad0c5e96bae Mon Sep 17 00:00:00 2001 From: Ralf Habacker Date: Tue, 3 Dec 2019 01:29:10 +0100 Subject: [PATCH 1/5] dbus_poll(): Remove debug output to make room for a better implementation --- dbus/dbus-sysdeps-win.c | 106 ---------------------------------------- 1 file changed, 106 deletions(-) diff --git a/dbus/dbus-sysdeps-win.c b/dbus/dbus-sysdeps-win.c index 964d5391..6c9c1d56 100644 --- a/dbus/dbus-sysdeps-win.c +++ b/dbus/dbus-sysdeps-win.c @@ -1175,10 +1175,6 @@ _dbus_poll (DBusPollFD *fds, #if USE_CHRIS_IMPL -#define DBUS_POLL_CHAR_BUFFER_SIZE 2000 - char msg[DBUS_POLL_CHAR_BUFFER_SIZE]; - char *msgp; - int ret = 0; int i; struct timeval tv; @@ -1192,34 +1188,6 @@ _dbus_poll (DBusPollFD *fds, else pEvents = eventsOnStack; - -#ifdef DBUS_ENABLE_VERBOSE_MODE - msgp = msg; - msgp += sprintf (msgp, "WSAEventSelect: to=%d\n\t", timeout_milliseconds); - for (i = 0; i < n_fds; i++) - { - DBusPollFD *fdp = &fds[i]; - - - if (fdp->events & _DBUS_POLLIN) - msgp += sprintf (msgp, "R:%Iu ", fdp->fd.sock); - - if (fdp->events & _DBUS_POLLOUT) - msgp += sprintf (msgp, "W:%Iu ", fdp->fd.sock); - - msgp += sprintf (msgp, "E:%Iu\n\t", fdp->fd.sock); - - // FIXME: more robust code for long msg - // create on heap when msg[] becomes too small - if (msgp >= msg + DBUS_POLL_CHAR_BUFFER_SIZE) - { - _dbus_assert_not_reached ("buffer overflow in _dbus_poll"); - } - } - - msgp += sprintf (msgp, "\n"); - _dbus_verbose ("%s",msg); -#endif for (i = 0; i < n_fds; i++) { DBusPollFD *fdp = &fds[i]; @@ -1239,7 +1207,6 @@ _dbus_poll (DBusPollFD *fds, pEvents[i] = ev; } - ready = WSAWaitForMultipleEvents (n_fds, pEvents, FALSE, timeout_milliseconds, FALSE); if (DBUS_SOCKET_API_RETURNS_ERROR (ready)) @@ -1256,9 +1223,6 @@ _dbus_poll (DBusPollFD *fds, } else if (ready >= WSA_WAIT_EVENT_0 && ready < (int)(WSA_WAIT_EVENT_0 + n_fds)) { - msgp = msg; - msgp += sprintf (msgp, "WSAWaitForMultipleEvents: =%d\n\t", ready); - for (i = 0; i < n_fds; i++) { DBusPollFD *fdp = &fds[i]; @@ -1277,25 +1241,11 @@ _dbus_poll (DBusPollFD *fds, if (ne.lNetworkEvents & (FD_OOB)) fdp->revents |= _DBUS_POLLERR; - if (ne.lNetworkEvents & (FD_READ | FD_ACCEPT | FD_CLOSE)) - msgp += sprintf (msgp, "R:%Iu ", fdp->fd.sock); - - if (ne.lNetworkEvents & (FD_WRITE | FD_CONNECT)) - msgp += sprintf (msgp, "W:%Iu ", fdp->fd.sock); - - if (ne.lNetworkEvents & (FD_OOB)) - msgp += sprintf (msgp, "E:%Iu ", fdp->fd.sock); - - msgp += sprintf (msgp, "lNetworkEvents:%d ", ne.lNetworkEvents); - if(ne.lNetworkEvents) ret++; WSAEventSelect(fdp->fd.sock, pEvents[i], 0); } - - msgp += sprintf (msgp, "\n"); - _dbus_verbose ("%s",msg); } else { @@ -1314,13 +1264,6 @@ _dbus_poll (DBusPollFD *fds, return ret; #else /* USE_CHRIS_IMPL */ - -#ifdef DBUS_ENABLE_VERBOSE_MODE -#define DBUS_POLL_CHAR_BUFFER_SIZE 2000 - char msg[DBUS_POLL_CHAR_BUFFER_SIZE]; - char *msgp; -#endif - fd_set read_set, write_set, err_set; SOCKET max_fd = 0; int i; @@ -1331,34 +1274,6 @@ _dbus_poll (DBusPollFD *fds, FD_ZERO (&write_set); FD_ZERO (&err_set); - -#ifdef DBUS_ENABLE_VERBOSE_MODE - msgp = msg; - msgp += sprintf (msgp, "select: to=%d\n\t", timeout_milliseconds); - for (i = 0; i < n_fds; i++) - { - DBusPollFD *fdp = &fds[i]; - - - if (fdp->events & _DBUS_POLLIN) - msgp += sprintf (msgp, "R:%Iu ", fdp->fd.sock); - - if (fdp->events & _DBUS_POLLOUT) - msgp += sprintf (msgp, "W:%Iu ", fdp->fd.sock); - - msgp += sprintf (msgp, "E:%Iu\n\t", fdp->fd.sock); - - // FIXME: more robust code for long msg - // create on heap when msg[] becomes too small - if (msgp >= msg + DBUS_POLL_CHAR_BUFFER_SIZE) - { - _dbus_assert_not_reached ("buffer overflow in _dbus_poll"); - } - } - - msgp += sprintf (msgp, "\n"); - _dbus_verbose ("%s",msg); -#endif for (i = 0; i < n_fds; i++) { DBusPollFD *fdp = &fds[i]; @@ -1391,27 +1306,6 @@ _dbus_poll (DBusPollFD *fds, else if (ready > 0) { -#ifdef DBUS_ENABLE_VERBOSE_MODE - msgp = msg; - msgp += sprintf (msgp, "select: = %d:\n\t", ready); - - for (i = 0; i < n_fds; i++) - { - DBusPollFD *fdp = &fds[i]; - - if (FD_ISSET (fdp->fd.sock, &read_set)) - msgp += sprintf (msgp, "R:%Iu ", fdp->fd.sock); - - if (FD_ISSET (fdp->fd.sock, &write_set)) - msgp += sprintf (msgp, "W:%Iu ", fdp->fd.sock); - - if (FD_ISSET (fdp->fd.sock, &err_set)) - msgp += sprintf (msgp, "E:%Iu\n\t", fdp->fd.sock); - } - msgp += sprintf (msgp, "\n"); - _dbus_verbose ("%s",msg); -#endif - for (i = 0; i < n_fds; i++) { DBusPollFD *fdp = &fds[i]; From 0d714aed9dd98c9664d86cbdeb0dda05e50ac45d Mon Sep 17 00:00:00 2001 From: Ralf Habacker Date: Tue, 28 Apr 2020 07:04:25 +0200 Subject: [PATCH 2/5] Separate the event based implementation for _dbus_poll() from the fd based one The function _dbus_poll() has been split into two functions, _dbus_poll_events() and _dbus_poll_select(), each containing the corresponding implementation. _dbus_poll() now calls the corresponding function. --- dbus/dbus-sysdeps-win.c | 68 +++++++++++++++++++++++++++++------------ 1 file changed, 48 insertions(+), 20 deletions(-) diff --git a/dbus/dbus-sysdeps-win.c b/dbus/dbus-sysdeps-win.c index 6c9c1d56..4c813af3 100644 --- a/dbus/dbus-sysdeps-win.c +++ b/dbus/dbus-sysdeps-win.c @@ -82,6 +82,8 @@ extern BOOL WINAPI ConvertSidToStringSidA (PSID Sid, LPSTR *StringSid); typedef int socklen_t; +/* uncomment to enable windows event based poll implementation */ +//#define USE_CHRIS_IMPL void _dbus_win_set_errno (int err) @@ -1158,26 +1160,22 @@ out0: return FALSE; } +#ifdef USE_CHRIS_IMPL /** - * Wrapper for poll(). + * Windows event based implementation for _dbus_poll(). * * @param fds the file descriptors to poll * @param n_fds number of descriptors in the array * @param timeout_milliseconds timeout or -1 for infinite * @returns numbers of fds with revents, or <0 on error */ -int -_dbus_poll (DBusPollFD *fds, - int n_fds, - int timeout_milliseconds) +static int +_dbus_poll_events (DBusPollFD *fds, + int n_fds, + int timeout_milliseconds) { -#define USE_CHRIS_IMPL 0 - -#if USE_CHRIS_IMPL - int ret = 0; int i; - struct timeval tv; int ready; #define DBUS_STACK_WSAEVENTS 256 @@ -1202,7 +1200,7 @@ _dbus_poll (DBusPollFD *fds, if (fdp->events & _DBUS_POLLOUT) lNetworkEvents |= FD_WRITE | FD_CONNECT; - WSAEventSelect(fdp->fd.sock, ev, lNetworkEvents); + WSAEventSelect (fdp->fd.sock, ev, lNetworkEvents); pEvents[i] = ev; } @@ -1230,7 +1228,7 @@ _dbus_poll (DBusPollFD *fds, fdp->revents = 0; - WSAEnumNetworkEvents(fdp->fd.sock, pEvents[i], &ne); + WSAEnumNetworkEvents (fdp->fd.sock, pEvents[i], &ne); if (ne.lNetworkEvents & (FD_READ | FD_ACCEPT | FD_CLOSE)) fdp->revents |= _DBUS_POLLIN; @@ -1244,7 +1242,7 @@ _dbus_poll (DBusPollFD *fds, if(ne.lNetworkEvents) ret++; - WSAEventSelect(fdp->fd.sock, pEvents[i], 0); + WSAEventSelect (fdp->fd.sock, pEvents[i], 0); } } else @@ -1255,15 +1253,28 @@ _dbus_poll (DBusPollFD *fds, for(i = 0; i < n_fds; i++) { - WSACloseEvent(pEvents[i]); + WSACloseEvent (pEvents[i]); } if (n_fds > DBUS_STACK_WSAEVENTS) - free(pEvents); + free (pEvents); return ret; - -#else /* USE_CHRIS_IMPL */ +} +#else +/** + * Select based implementation for _dbus_poll(). + * + * @param fds the file descriptors to poll + * @param n_fds number of descriptors in the array + * @param timeout_milliseconds timeout or -1 for infinite + * @returns numbers of fds with revents, or <0 on error + */ +static int +_dbus_poll_select (DBusPollFD *fds, + int n_fds, + int timeout_milliseconds) +{ fd_set read_set, write_set, err_set; SOCKET max_fd = 0; int i; @@ -1323,11 +1334,28 @@ _dbus_poll (DBusPollFD *fds, } } return ready; -#endif /* USE_CHRIS_IMPL */ } +#endif - - +/** + * Wrapper for poll(). + * + * @param fds the file descriptors to poll + * @param n_fds number of descriptors in the array + * @param timeout_milliseconds timeout or -1 for infinite + * @returns numbers of fds with revents, or <0 on error + */ +int +_dbus_poll (DBusPollFD *fds, + int n_fds, + int timeout_milliseconds) +{ +#ifdef USE_CHRIS_IMPL + return _dbus_poll_events (fds, n_fds, timeout_milliseconds); +#else + return _dbus_poll_select (fds, n_fds, timeout_milliseconds); +#endif +} /****************************************************************************** From 96b829583125c39a88890989392948b33bdff8cc Mon Sep 17 00:00:00 2001 From: Ralf Habacker Date: Tue, 28 Apr 2020 07:02:09 +0200 Subject: [PATCH 3/5] Fix bug not detecting out of memory condition in _dbus_poll_events () For cleaning purpose the event list members are initialized with WSA_INVALID_EVENT. The cleanup code detects and handles the case that the event list has been created from calloc (). --- dbus/dbus-sysdeps-win.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/dbus/dbus-sysdeps-win.c b/dbus/dbus-sysdeps-win.c index 4c813af3..2a1a86ea 100644 --- a/dbus/dbus-sysdeps-win.c +++ b/dbus/dbus-sysdeps-win.c @@ -1186,6 +1186,16 @@ _dbus_poll_events (DBusPollFD *fds, else pEvents = eventsOnStack; + if (pEvents == NULL) + { + _dbus_win_set_errno (ENOMEM); + ret = -1; + goto oom; + } + + for (i = 0; i < n_fds; i++) + pEvents[i] = WSA_INVALID_EVENT; + for (i = 0; i < n_fds; i++) { DBusPollFD *fdp = &fds[i]; @@ -1251,14 +1261,18 @@ _dbus_poll_events (DBusPollFD *fds, ret = -1; } - for(i = 0; i < n_fds; i++) +oom: + if (pEvents != NULL) { - WSACloseEvent (pEvents[i]); + for (i = 0; i < n_fds; i++) + { + if (pEvents[i] != WSA_INVALID_EVENT) + WSACloseEvent (pEvents[i]); + } + if (n_fds > DBUS_STACK_WSAEVENTS) + free (pEvents); } - if (n_fds > DBUS_STACK_WSAEVENTS) - free (pEvents); - return ret; } #else From b09ba846aacb281b5dfd3ccce01b9a057ca9231e Mon Sep 17 00:00:00 2001 From: Ralf Habacker Date: Tue, 28 Apr 2020 07:06:41 +0200 Subject: [PATCH 4/5] Add debug output functions for _dbus_poll_xx() functions --- dbus/dbus-sysdeps-win.c | 148 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 148 insertions(+) diff --git a/dbus/dbus-sysdeps-win.c b/dbus/dbus-sysdeps-win.c index 2a1a86ea..e75b87cf 100644 --- a/dbus/dbus-sysdeps-win.c +++ b/dbus/dbus-sysdeps-win.c @@ -1160,6 +1160,117 @@ out0: return FALSE; } +#ifdef DBUS_ENABLE_VERBOSE_MODE +static dbus_bool_t +_dbus_dump_fd_events (DBusPollFD *fds, int n_fds) +{ + DBusString msg = _DBUS_STRING_INIT_INVALID; + dbus_bool_t result = FALSE; + int i; + + if (!_dbus_string_init (&msg)) + goto oom; + + for (i = 0; i < n_fds; i++) + { + DBusPollFD *fdp = &fds[i]; + if (!_dbus_string_append (&msg, i > 0 ? "\n\t" : "\t")) + goto oom; + + if ((fdp->events & _DBUS_POLLIN) && + !_dbus_string_append_printf (&msg, "R:%Iu ", fdp->fd.sock)) + goto oom; + + if ((fdp->events & _DBUS_POLLOUT) && + !_dbus_string_append_printf (&msg, "W:%Iu ", fdp->fd.sock)) + goto oom; + + if (!_dbus_string_append_printf (&msg, "E:%Iu", fdp->fd.sock)) + goto oom; + } + + _dbus_verbose ("%s\n", _dbus_string_get_const_data (&msg)); + result = TRUE; +oom: + _dbus_string_free (&msg); + return result; +} + +#ifdef USE_CHRIS_IMPL +static dbus_bool_t +_dbus_dump_fd_revents (DBusPollFD *fds, int n_fds) +{ + DBusString msg = _DBUS_STRING_INIT_INVALID; + dbus_bool_t result = FALSE; + int i; + + if (!_dbus_string_init (&msg)) + goto oom; + + for (i = 0; i < n_fds; i++) + { + DBusPollFD *fdp = &fds[i]; + if (!_dbus_string_append (&msg, i > 0 ? "\n\t" : "\t")) + goto oom; + + if ((fdp->revents & _DBUS_POLLIN) && + !_dbus_string_append_printf (&msg, "R:%Iu ", fdp->fd.sock)) + goto oom; + + if ((fdp->revents & _DBUS_POLLOUT) && + !_dbus_string_append_printf (&msg, "W:%Iu ", fdp->fd.sock)) + goto oom; + + if ((fdp->revents & _DBUS_POLLERR) && + !_dbus_string_append_printf (&msg, "E:%Iu", fdp->fd.sock)) + goto oom; + } + + _dbus_verbose ("%s\n", _dbus_string_get_const_data (&msg)); + result = TRUE; +oom: + _dbus_string_free (&msg); + return result; +} +#else +static dbus_bool_t +_dbus_dump_fdset (DBusPollFD *fds, int n_fds, fd_set *read_set, fd_set *write_set, fd_set *err_set) +{ + DBusString msg = _DBUS_STRING_INIT_INVALID; + dbus_bool_t result = FALSE; + int i; + + if (!_dbus_string_init (&msg)) + goto oom; + + for (i = 0; i < n_fds; i++) + { + DBusPollFD *fdp = &fds[i]; + + if (!_dbus_string_append (&msg, i > 0 ? "\n\t" : "\t")) + goto oom; + + if (FD_ISSET (fdp->fd.sock, read_set) && + !_dbus_string_append_printf (&msg, "R:%Iu ", fdp->fd.sock)) + goto oom; + + if (FD_ISSET (fdp->fd.sock, write_set) && + !_dbus_string_append_printf (&msg, "W:%Iu ", fdp->fd.sock)) + goto oom; + + if (FD_ISSET (fdp->fd.sock, err_set) && + !_dbus_string_append_printf (&msg, "E:%Iu", fdp->fd.sock)) + goto oom; + } + _dbus_verbose ("%s\n", _dbus_string_get_const_data (&msg)); + result = TRUE; +oom: + _dbus_string_free (&msg); + return result; +} +#endif +#endif + #ifdef USE_CHRIS_IMPL /** * Windows event based implementation for _dbus_poll(). @@ -1193,6 +1304,16 @@ _dbus_poll_events (DBusPollFD *fds, goto oom; } +#ifdef DBUS_ENABLE_VERBOSE_MODE + _dbus_verbose ("_dbus_poll: to=%d", timeout_milliseconds); + if (!_dbus_dump_fd_events (fds, n_fds)) + { + _dbus_win_set_errno (ENOMEM); + ret = -1; + goto oom; + } +#endif + for (i = 0; i < n_fds; i++) pEvents[i] = WSA_INVALID_EVENT; @@ -1254,6 +1375,15 @@ _dbus_poll_events (DBusPollFD *fds, WSAEventSelect (fdp->fd.sock, pEvents[i], 0); } +#ifdef DBUS_ENABLE_VERBOSE_MODE + _dbus_verbose ("_dbus_poll: to=%d", timeout_milliseconds); + if (!_dbus_dump_fd_revents (fds, n_fds)) + { + _dbus_win_set_errno (ENOMEM); + ret = -1; + goto oom; + } +#endif } else { @@ -1298,6 +1428,14 @@ _dbus_poll_select (DBusPollFD *fds, FD_ZERO (&read_set); FD_ZERO (&write_set); FD_ZERO (&err_set); +#ifdef DBUS_ENABLE_VERBOSE_MODE + _dbus_verbose("_dbus_poll: to=%d", timeout_milliseconds); + if (!_dbus_dump_fd_events (fds, n_fds)) + { + ready = -1; + goto oom; + } +#endif for (i = 0; i < n_fds; i++) { @@ -1331,6 +1469,15 @@ _dbus_poll_select (DBusPollFD *fds, else if (ready > 0) { +#ifdef DBUS_ENABLE_VERBOSE_MODE + _dbus_verbose ("select: to=%d\n", ready); + if (!_dbus_dump_fdset (fds, n_fds, &read_set, &write_set, &err_set)) + { + _dbus_win_set_errno (ENOMEM); + ready = -1; + goto oom; + } +#endif for (i = 0; i < n_fds; i++) { DBusPollFD *fdp = &fds[i]; @@ -1347,6 +1494,7 @@ _dbus_poll_select (DBusPollFD *fds, fdp->revents |= _DBUS_POLLERR; } } +oom: return ready; } #endif From 2658b2571c9792a4c3924b42a4f2b038b56bbcd3 Mon Sep 17 00:00:00 2001 From: Ralf Habacker Date: Tue, 3 Dec 2019 02:23:29 +0100 Subject: [PATCH 5/5] Fix return type and usage of WSAWaitForMultipleEvents() The former int type leads to warnings. --- dbus/dbus-sysdeps-win.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dbus/dbus-sysdeps-win.c b/dbus/dbus-sysdeps-win.c index e75b87cf..5d9cae73 100644 --- a/dbus/dbus-sysdeps-win.c +++ b/dbus/dbus-sysdeps-win.c @@ -1287,7 +1287,7 @@ _dbus_poll_events (DBusPollFD *fds, { int ret = 0; int i; - int ready; + DWORD ready; #define DBUS_STACK_WSAEVENTS 256 WSAEVENT eventsOnStack[DBUS_STACK_WSAEVENTS]; @@ -1338,7 +1338,7 @@ _dbus_poll_events (DBusPollFD *fds, ready = WSAWaitForMultipleEvents (n_fds, pEvents, FALSE, timeout_milliseconds, FALSE); - if (DBUS_SOCKET_API_RETURNS_ERROR (ready)) + if (ready == WSA_WAIT_FAILED) { DBUS_SOCKET_SET_ERRNO (); if (errno != WSAEWOULDBLOCK) @@ -1350,7 +1350,7 @@ _dbus_poll_events (DBusPollFD *fds, _dbus_verbose ("WSAWaitForMultipleEvents: WSA_WAIT_TIMEOUT\n"); ret = 0; } - else if (ready >= WSA_WAIT_EVENT_0 && ready < (int)(WSA_WAIT_EVENT_0 + n_fds)) + else if (ready < (WSA_WAIT_EVENT_0 + n_fds)) { for (i = 0; i < n_fds; i++) {