The test also calls GetBestRoute2 with only the smaller sockaddr_in but reads inside the whole SOCKADDR_INET type.
Therefore I recived this ASan report, reading 28 bytes from variable with only 16 bytes.
The second patch removes a few unneeded casts to SOCKADDR_INET.
But there are more which ASan does not complain about here, should they also be changed to a SOCKADDR_INET type?
CC: @yshui , @gofman
Followup to 93a3d8bf9f (!9085) / ada093a6d2 (!8991).
<details> <summary>ASan details:</summary>
``` ================================================================= ==iphlpapi_test.exe==560==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffffe1ff440 at pc 0x6ffffe86c02b bp 0x7ffffe1fe630 sp 0x7ffffe1fe678 READ of size 28 at 0x7ffffe1ff440 thread T0 #0 0x6ffffe86c02a in __asan_memcpy /home/runner/work/llvm-mingw/llvm-mingw/llvm-project/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp:65:3 #1 0x6ffffb1b869a in GetBestRoute2 .../wine/dlls/iphlpapi/iphlpapi_main.c:4851:20 #2 0x00014000e0b2 in test_best_routes .../wine/dlls/iphlpapi/tests/iphlpapi.c:3908:11 #3 0x00014000123f in func_iphlpapi .../wine/dlls/iphlpapi/tests/iphlpapi.c:4017:5 #4 0x000140022bb2 in run_test .../wine/include/wine/test.h:765:5 #5 0x00014002262c in main .../wine/include/wine/test.h #6 0x0001400247ba in mainCRTStartup .../wine/dlls/msvcrt/crt_main.c:62:11 #7 0x6fffffc377b0 in BaseThreadInitThunk .../wine/dlls/kernel32/thread.c:61:24 #8 0x6fffffdc05b6 in RtlUserThreadStart (C:\windows\system32\ntdll.dll+0x1700505b6)
Address 0x7ffffe1ff440 is located in stack of thread T0 at offset 384 in frame #0 0x00014000b3cf in test_best_routes .../wine/dlls/iphlpapi/tests/iphlpapi.c:3503
This frame has 17 object(s): [32, 36) 'index' (line 3506) [48, 76) 'dst6' (line 3507) [112, 140) 'src6' (line 3507) [176, 204) 'best6' (line 3507) [240, 268) 'link_local_addr6' (line 3507) [304, 332) 'global_addr6' (line 3507) [368, 384) 'dst4' (line 3508) [400, 416) 'src4' (line 3508) <== Memory access at offset 384 partially underflows this variable [432, 448) 'global_addr4' (line 3508) [464, 472) 'link_local_luid' (line 3509) [496, 504) 'luid' (line 3509) [528, 608) 'uni_row' (line 3510) [640, 744) 'fwd_row' (line 3511) [784, 792) 'table' (line 3512) [816, 1072) 's' (line 3513) [1136, 1392) 's2' (line 3513) [1456, 1484) 'best4' (line 3514) HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork (longjmp, SEH and C++ exceptions *are* supported) SUMMARY: AddressSanitizer: stack-buffer-overflow .../wine/dlls/iphlpapi/iphlpapi_main.c:4851:20 in GetBestRoute2 Shadow bytes around the buggy address: 0x7ffffe1ff180: f2 f2 f2 f2 04 f3 f3 f3 00 00 00 00 00 00 00 00 0x7ffffe1ff200: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x7ffffe1ff280: 00 00 00 00 00 00 00 00 f1 f1 f1 f1 04 f2 00 00 0x7ffffe1ff300: 00 04 f2 f2 f2 f2 00 00 00 04 f2 f2 f2 f2 00 00 0x7ffffe1ff380: 00 04 f2 f2 f2 f2 00 00 00 04 f2 f2 f2 f2 00 00 =>0x7ffffe1ff400: 00 04 f2 f2 f2 f2 00 00[f2]f2 00 00 f2 f2 00 00 0x7ffffe1ff480: f2 f2 00 f2 f2 f2 00 f2 f2 f2 00 00 00 00 00 00 0x7ffffe1ff500: 00 00 00 00 f2 f2 f2 f2 00 00 00 00 00 00 00 00 0x7ffffe1ff580: 00 00 00 00 00 f2 f2 f2 f2 f2 00 f2 f2 f2 00 00 0x7ffffe1ff600: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x7ffffe1ff680: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 f2 f2 Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb
Thread 1 "0204" hit Breakpoint 5, GetBestRoute2 (luid=<optimized out>, luid@entry=0x0, index=index@entry=2, src=src@entry=0x0, dst=dst@entry=0x7ffffe2ff7b0, options=<optimized out>, options@entry=0, bestroute=<optimized out>, bestroute@entry=0x7ffffe2ff8f0, bestaddress=<optimized out>, bestaddress@entry=0x7ffffe2ff880) at .../wine/dlls/iphlpapi/iphlpapi_main.c:4851 4851 dst_addr = *dst; Wine-gdb> bt #0 GetBestRoute2 (luid=<optimized out>, luid@entry=0x0, index=index@entry=2, src=src@entry=0x0, dst=dst@entry=0x7ffffe2ff7b0, options=<optimized out>, options@entry=0, bestroute=<optimized out>, bestroute@entry=0x7ffffe2ff8f0, bestaddress=<optimized out>, bestaddress@entry=0x7ffffe2ff880) at .../wine/dlls/iphlpapi/iphlpapi_main.c:4851 #1 0x0000000140004e57 in test_best_routes () at .../wine/dlls/iphlpapi/tests/iphlpapi.c:3908 #2 func_iphlpapi () at .../wine/dlls/iphlpapi/tests/iphlpapi.c:4017 #3 0x00000001400061dd in run_test (name=name@entry=0x14001b21b <winetest_color_reset+189> "iphlpapi") at .../wine/include/wine/test.h:765 #4 0x0000000140005ec4 in main (argc=<optimized out>, argv=<optimized out>) at .../wine/include/wine/test.h:877 Wine-gdb> print dst $1 = (const SOCKADDR_INET *) 0x7ffffe2ff7b0 Wine-gdb> print sizeof(*dst) $2 = 28 Wine-gdb> up #1 0x0000000140004e57 in test_best_routes () at .../wine/dlls/iphlpapi/tests/iphlpapi.c:3908 3908 ret = GetBestRoute2( NULL, default_route_index, NULL, (SOCKADDR_INET *)&dst4, 0, &fwd_row, (SOCKADDR_INET *)&best4 ); Wine-gdb> print &dst4 $3 = (struct sockaddr_in *) 0x7ffffe2ff7b0 Wine-gdb> print sizeof(dst4) $4 = 16 Wine-gdb> ```
</details>
From: Bernhard Übelacker bernhardu@mailbox.org
The test also calls GetBestRoute2 with only the smaller sockaddr_in but reads inside the whole SOCKADDR_INET type.
Followup to 93a3d8bf9f / ada093a6d2. --- dlls/iphlpapi/tests/iphlpapi.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/dlls/iphlpapi/tests/iphlpapi.c b/dlls/iphlpapi/tests/iphlpapi.c index 49c3059b5df..cc7d05346d2 100644 --- a/dlls/iphlpapi/tests/iphlpapi.c +++ b/dlls/iphlpapi/tests/iphlpapi.c @@ -3505,7 +3505,8 @@ static void test_best_routes(void)
DWORD ret, index, link_local_index = ~0u, loopback_index = ~0u, default_route_index = ~0u; struct sockaddr_in6 dst6, src6, best6, link_local_addr6, global_addr6; - struct sockaddr_in dst4, src4, global_addr4; + struct sockaddr_in src4, global_addr4; + SOCKADDR_INET dst4; NET_LUID link_local_luid = { 0 }, luid; MIB_UNICASTIPADDRESS_ROW uni_row; MIB_IPFORWARD_ROW2 fwd_row, *r; @@ -3890,8 +3891,8 @@ loopback_ipv6:
/* Test with ipv4 */ memset( &dst4, 0xcc, sizeof(dst4) ); - dst4.sin_family = AF_INET; - ret = inet_pton( AF_INET, "25.25.0.0", &dst4.sin_addr); + dst4.Ipv4.sin_family = AF_INET; + ret = inet_pton( AF_INET, "25.25.0.0", &dst4.Ipv4.sin_addr); ok(ret, "got error %u.\n", WSAGetLastError());
ret = GetBestInterfaceEx( (struct sockaddr *)&dst4, &index ); @@ -3905,7 +3906,7 @@ loopback_ipv6:
memset( &fwd_row, 0xcc, sizeof(fwd_row) ); memset( &best4, 0xcc, sizeof(best4) ); - ret = GetBestRoute2( NULL, default_route_index, NULL, (SOCKADDR_INET *)&dst4, 0, &fwd_row, (SOCKADDR_INET *)&best4 ); + ret = GetBestRoute2( NULL, default_route_index, NULL, &dst4, 0, &fwd_row, (SOCKADDR_INET *)&best4 ); ok( !ret, "got error %lu.\n", ret ); ok( fwd_row.InterfaceIndex == default_route_index, "got %lu, expected %lu.\n", fwd_row.InterfaceIndex, default_route_index ); @@ -3926,13 +3927,13 @@ loopback_ipv6: ret = inet_pton( AF_INET, "127.0.0.1", &src4.sin_addr ); ok(ret, "got error %u.\n", WSAGetLastError()); memset( &best4, 0xcc, sizeof(best4) ); - ret = GetBestRoute2( NULL, 0, (SOCKADDR_INET *)&src4, (SOCKADDR_INET *)&dst4, 0, &fwd_row, (SOCKADDR_INET *)&best4 ); + ret = GetBestRoute2( NULL, 0, (SOCKADDR_INET *)&src4, &dst4, 0, &fwd_row, (SOCKADDR_INET *)&best4 ); ok( ret == ERROR_NETWORK_UNREACHABLE, "got error %lu.\n", ret ); ok( !best4.Ipv4.sin_family, "got %u.\n", best4.Ipv4.sin_family ); ok( !best4.Ipv6.sin6_addr.u.Word[7], "got %#x.\n", best4.Ipv6.sin6_addr.u.Word[7] );
src4 = global_addr4; - ret = GetBestRoute2( NULL, loopback_index, (SOCKADDR_INET *)&src4, (SOCKADDR_INET *)&dst4, 0, &fwd_row, + ret = GetBestRoute2( NULL, loopback_index, (SOCKADDR_INET *)&src4, &dst4, 0, &fwd_row, (SOCKADDR_INET *)&best4 ); ok( !ret, "got error %lu.\n", ret ); ok( fwd_row.InterfaceIndex == default_route_index, "got %lu, expected %lu.\n", fwd_row.InterfaceIndex, @@ -3942,29 +3943,29 @@ loopback_ipv6:
memset( &src4, 0xcc, sizeof(src4) ); memset( &fwd_row, 0xcc, sizeof(fwd_row) ); - ret = GetBestRoute2( NULL, default_route_index, (SOCKADDR_INET *)&src4, (SOCKADDR_INET *)&dst4, 0, &fwd_row, + ret = GetBestRoute2( NULL, default_route_index, (SOCKADDR_INET *)&src4, &dst4, 0, &fwd_row, (SOCKADDR_INET *)&best4 ); ok( !ret, "got error %lu.\n", ret ); ok( fwd_row.InterfaceIndex == default_route_index, "got %lu, expected %lu.\n", fwd_row.InterfaceIndex, link_local_index );
src4 = best4.Ipv4; - ret = GetBestRoute2( NULL, default_route_index, (SOCKADDR_INET *)&src4, (SOCKADDR_INET *)&dst4, 0, &fwd_row, + ret = GetBestRoute2( NULL, default_route_index, (SOCKADDR_INET *)&src4, &dst4, 0, &fwd_row, (SOCKADDR_INET *)&best4 ); ok( !ret, "got error %lu.\n", ret ); ok( fwd_row.InterfaceIndex == default_route_index, "got %lu, expected %lu.\n", fwd_row.InterfaceIndex, link_local_index ); ok( fwd_row.NextHop.Ipv4.sin_addr.s_addr, "expected specified next hop.\n" );
- ret = GetBestRoute2( NULL, loopback_index, NULL, (SOCKADDR_INET *)&dst4, 0, &fwd_row, (SOCKADDR_INET *)&best4 ); + ret = GetBestRoute2( NULL, loopback_index, NULL, &dst4, 0, &fwd_row, (SOCKADDR_INET *)&best4 ); ok( ret == ERROR_NETWORK_UNREACHABLE, "got error %lu.\n", ret );
- ret = inet_pton( AF_INET, "127.0.0.1", &dst4.sin_addr); + ret = inet_pton( AF_INET, "127.0.0.1", &dst4.Ipv4.sin_addr); ok(ret, "got error %u.\n", WSAGetLastError()); - ret = GetBestRoute2( NULL, default_route_index, NULL, (SOCKADDR_INET *)&dst4, 0, &fwd_row, (SOCKADDR_INET *)&best4 ); + ret = GetBestRoute2( NULL, default_route_index, NULL, &dst4, 0, &fwd_row, (SOCKADDR_INET *)&best4 ); ok( ret == ERROR_INVALID_PARAMETER, "got error %lu.\n", ret );
- ret = GetBestRoute2( NULL, 0, NULL, (SOCKADDR_INET *)&dst4, 0, &fwd_row, (SOCKADDR_INET *)&best4 ); + ret = GetBestRoute2( NULL, 0, NULL, &dst4, 0, &fwd_row, (SOCKADDR_INET *)&best4 ); ok( !ret, "got error %lu.\n", ret ); ok( !fwd_row.NextHop.Ipv4.sin_addr.s_addr, "expected unspecified next hop.\n" );
From: Bernhard Übelacker bernhardu@mailbox.org
--- dlls/iphlpapi/tests/iphlpapi.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/dlls/iphlpapi/tests/iphlpapi.c b/dlls/iphlpapi/tests/iphlpapi.c index cc7d05346d2..b67ef0e7d8e 100644 --- a/dlls/iphlpapi/tests/iphlpapi.c +++ b/dlls/iphlpapi/tests/iphlpapi.c @@ -3906,7 +3906,7 @@ loopback_ipv6:
memset( &fwd_row, 0xcc, sizeof(fwd_row) ); memset( &best4, 0xcc, sizeof(best4) ); - ret = GetBestRoute2( NULL, default_route_index, NULL, &dst4, 0, &fwd_row, (SOCKADDR_INET *)&best4 ); + ret = GetBestRoute2( NULL, default_route_index, NULL, &dst4, 0, &fwd_row, &best4 ); ok( !ret, "got error %lu.\n", ret ); ok( fwd_row.InterfaceIndex == default_route_index, "got %lu, expected %lu.\n", fwd_row.InterfaceIndex, default_route_index ); @@ -3927,14 +3927,14 @@ loopback_ipv6: ret = inet_pton( AF_INET, "127.0.0.1", &src4.sin_addr ); ok(ret, "got error %u.\n", WSAGetLastError()); memset( &best4, 0xcc, sizeof(best4) ); - ret = GetBestRoute2( NULL, 0, (SOCKADDR_INET *)&src4, &dst4, 0, &fwd_row, (SOCKADDR_INET *)&best4 ); + ret = GetBestRoute2( NULL, 0, (SOCKADDR_INET *)&src4, &dst4, 0, &fwd_row, &best4 ); ok( ret == ERROR_NETWORK_UNREACHABLE, "got error %lu.\n", ret ); ok( !best4.Ipv4.sin_family, "got %u.\n", best4.Ipv4.sin_family ); ok( !best4.Ipv6.sin6_addr.u.Word[7], "got %#x.\n", best4.Ipv6.sin6_addr.u.Word[7] );
src4 = global_addr4; ret = GetBestRoute2( NULL, loopback_index, (SOCKADDR_INET *)&src4, &dst4, 0, &fwd_row, - (SOCKADDR_INET *)&best4 ); + &best4 ); ok( !ret, "got error %lu.\n", ret ); ok( fwd_row.InterfaceIndex == default_route_index, "got %lu, expected %lu.\n", fwd_row.InterfaceIndex, link_local_index ); @@ -3944,28 +3944,28 @@ loopback_ipv6: memset( &src4, 0xcc, sizeof(src4) ); memset( &fwd_row, 0xcc, sizeof(fwd_row) ); ret = GetBestRoute2( NULL, default_route_index, (SOCKADDR_INET *)&src4, &dst4, 0, &fwd_row, - (SOCKADDR_INET *)&best4 ); + &best4 ); ok( !ret, "got error %lu.\n", ret ); ok( fwd_row.InterfaceIndex == default_route_index, "got %lu, expected %lu.\n", fwd_row.InterfaceIndex, link_local_index );
src4 = best4.Ipv4; ret = GetBestRoute2( NULL, default_route_index, (SOCKADDR_INET *)&src4, &dst4, 0, &fwd_row, - (SOCKADDR_INET *)&best4 ); + &best4 ); ok( !ret, "got error %lu.\n", ret ); ok( fwd_row.InterfaceIndex == default_route_index, "got %lu, expected %lu.\n", fwd_row.InterfaceIndex, link_local_index ); ok( fwd_row.NextHop.Ipv4.sin_addr.s_addr, "expected specified next hop.\n" );
- ret = GetBestRoute2( NULL, loopback_index, NULL, &dst4, 0, &fwd_row, (SOCKADDR_INET *)&best4 ); + ret = GetBestRoute2( NULL, loopback_index, NULL, &dst4, 0, &fwd_row, &best4 ); ok( ret == ERROR_NETWORK_UNREACHABLE, "got error %lu.\n", ret );
ret = inet_pton( AF_INET, "127.0.0.1", &dst4.Ipv4.sin_addr); ok(ret, "got error %u.\n", WSAGetLastError()); - ret = GetBestRoute2( NULL, default_route_index, NULL, &dst4, 0, &fwd_row, (SOCKADDR_INET *)&best4 ); + ret = GetBestRoute2( NULL, default_route_index, NULL, &dst4, 0, &fwd_row, &best4 ); ok( ret == ERROR_INVALID_PARAMETER, "got error %lu.\n", ret );
- ret = GetBestRoute2( NULL, 0, NULL, &dst4, 0, &fwd_row, (SOCKADDR_INET *)&best4 ); + ret = GetBestRoute2( NULL, 0, NULL, &dst4, 0, &fwd_row, &best4 ); ok( !ret, "got error %lu.\n", ret ); ok( !fwd_row.NextHop.Ipv4.sin_addr.s_addr, "expected unspecified next hop.\n" );
Huw Davies (@huw) commented about dlls/iphlpapi/tests/iphlpapi.c:
DWORD ret, index, link_local_index = ~0u, loopback_index = ~0u, default_route_index = ~0u; struct sockaddr_in6 dst6, src6, best6, link_local_addr6, global_addr6;
- struct sockaddr_in dst4, src4, global_addr4;
- struct sockaddr_in src4, global_addr4;
- SOCKADDR_INET dst4; NET_LUID link_local_luid = { 0 }, luid; MIB_UNICASTIPADDRESS_ROW uni_row; MIB_IPFORWARD_ROW2 fwd_row, *r; MIB_IPFORWARD_TABLE2 *table; char s[256], s2[256]; SOCKADDR_INET best4;
Could we declare `dst4` on the same line as `best4` please?
-
Bernhard Übelacker -
Bernhard Übelacker (＠bernhardu)
-
Huw Davies (＠huw)