[PATCH 0/2] MR5842: ws2_32/tests: Test fromlen for recvfrom().
Test that specifying a too small or too large fromlen for recvfrom() shouldn't write unnecessary data. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5842
From: Zhiyi Zhang <zzhang(a)codeweavers.com> Test that specifying a too small or too large fromlen for recvfrom() shouldn't write unnecessary data. --- dlls/ws2_32/tests/sock.c | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index 6954b3e077a..57f1e6f9f97 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -3185,7 +3185,7 @@ static void test_UDP(void) /* peer 0 receives data from all other peers */ static const TIMEVAL timeout_zero = {0}; struct sock_info peer[NUM_UDP_PEERS]; - char buf[16]; + char buf[16], sockaddr_buf[1024]; int ss, i, n_recv, n_sent, ret; struct sockaddr_in addr; int sock; @@ -3215,11 +3215,40 @@ static void test_UDP(void) ok ( peer[i].addr.sin_port != htons ( 0 ), "UDP: bind() did not associate port\n" ); } + /* Test that specifying a too small fromlen for recvfrom() shouldn't write unnecessary data */ + n_sent = sendto ( peer[1].s, buf, sizeof(buf), 0, (struct sockaddr *)&peer[0].addr, sizeof(peer[0].addr) ); + ok ( n_sent == sizeof(buf), "UDP: sendto() sent wrong amount of data or socket error: %d\n", n_sent ); + + sockaddr_buf[0] = 'A'; + ss = 1; + n_recv = recvfrom ( peer[0].s, buf, sizeof(buf), 0, (struct sockaddr *)sockaddr_buf, &ss ); + todo_wine + ok ( n_recv == SOCKET_ERROR, "UDP: recvfrom() succeeded\n" ); + todo_wine + ok ( sockaddr_buf[0] == 'A', "UDP: marker got overwritten\n" ); + if ( n_recv == SOCKET_ERROR ) + { + ss = sizeof ( peer[0].addr ); + n_recv = recvfrom ( peer[0].s, buf, sizeof(buf), 0, (struct sockaddr *)sockaddr_buf, &ss ); + ok ( n_recv == sizeof(buf), "UDP: recvfrom() failed\n" ); + } + + /* Test that specifying a large fromlen for recvfrom() shouldn't write unnecessary data besides the socket address */ + n_sent = sendto ( peer[1].s, buf, sizeof(buf), 0, (struct sockaddr *)&peer[0].addr, sizeof(peer[0].addr) ); + ok ( n_sent == sizeof(buf), "UDP: sendto() sent wrong amount of data or socket error: %d\n", n_sent ); + + sockaddr_buf[1023] = 'B'; + ss = sizeof(sockaddr_buf); + n_recv = recvfrom ( peer[0].s, buf, sizeof(buf), 0, (struct sockaddr *)sockaddr_buf, &ss ); + ok ( n_recv == sizeof(buf), "UDP: recvfrom() received wrong amount of data or socket error: %d\n", n_recv ); + todo_wine + ok ( sockaddr_buf[1023] == 'B', "UDP: marker got overwritten\n" ); + /* test getsockname() */ ok ( peer[0].addr.sin_port == htons ( SERVERPORT ), "UDP: getsockname returned incorrect peer port\n" ); for ( i = 1; i < NUM_UDP_PEERS; i++ ) { - /* send client's ip */ + /* send client's port */ memcpy( buf, &peer[i].addr.sin_port, sizeof(peer[i].addr.sin_port) ); n_sent = sendto ( peer[i].s, buf, sizeof(buf), 0, (struct sockaddr*) &peer[0].addr, sizeof(peer[0].addr) ); ok ( n_sent == sizeof(buf), "UDP: sendto() sent wrong amount of data or socket error: %d\n", n_sent ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5842
From: Zhiyi Zhang <zzhang(a)codeweavers.com> tallygatewayserver.exe specifies a from sockaddr pointing to a heap buffer smaller than 128 bytes yet it passes 128 as the fromlen to recvfrom(). So the memset(wsaddr, 0, wsaddrlen) call in sockaddr_from_unix() ends up trashing other data in the heap, causing the application to crash. Although this is an application bug, tests on Windows also showed that the socket address buffer should be written only with the necessary socket address data, thus preventing the crash. --- dlls/ntdll/unix/socket.c | 2 -- dlls/ws2_32/tests/sock.c | 2 -- 2 files changed, 4 deletions(-) diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c index 65f30759db3..ea2f5d3a670 100644 --- a/dlls/ntdll/unix/socket.c +++ b/dlls/ntdll/unix/socket.c @@ -298,8 +298,6 @@ static socklen_t sockaddr_to_unix( const struct WS_sockaddr *wsaddr, int wsaddrl static int sockaddr_from_unix( const union unix_sockaddr *uaddr, struct WS_sockaddr *wsaddr, socklen_t wsaddrlen ) { - memset( wsaddr, 0, wsaddrlen ); - switch (uaddr->addr.sa_family) { case AF_INET: diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index 57f1e6f9f97..e8c618cc0af 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -3224,7 +3224,6 @@ static void test_UDP(void) n_recv = recvfrom ( peer[0].s, buf, sizeof(buf), 0, (struct sockaddr *)sockaddr_buf, &ss ); todo_wine ok ( n_recv == SOCKET_ERROR, "UDP: recvfrom() succeeded\n" ); - todo_wine ok ( sockaddr_buf[0] == 'A', "UDP: marker got overwritten\n" ); if ( n_recv == SOCKET_ERROR ) { @@ -3241,7 +3240,6 @@ static void test_UDP(void) ss = sizeof(sockaddr_buf); n_recv = recvfrom ( peer[0].s, buf, sizeof(buf), 0, (struct sockaddr *)sockaddr_buf, &ss ); ok ( n_recv == sizeof(buf), "UDP: recvfrom() received wrong amount of data or socket error: %d\n", n_recv ); - todo_wine ok ( sockaddr_buf[1023] == 'B', "UDP: marker got overwritten\n" ); /* test getsockname() */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5842
Yeah, I have no idea why I wrote that :x -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5842#note_73106
This merge request was approved by Elizabeth Figura. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5842
participants (3)
-
Elizabeth Figura (＠zfigura) -
Zhiyi Zhang
-
Zhiyi Zhang (＠zhiyi)