[PATCH v2 0/2] MR1978: nsi: Fix two NsiAllocateAndGetTable() bugs
-- v2: nsi: Add some margin to the tables size in NsiAllocateAndGetTable(). nsi: Return an error if NsiAllocateAndGetTable() fails to allocate the tables. https://gitlab.winehq.org/wine/wine/-/merge_requests/1978
v2: only set err in the first commit; reword the comment in the second commit -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1978#note_21139
From: Francois Gouget <fgouget(a)codeweavers.com> err must be set if the last attempt to allocate the tables fails. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=54328 --- dlls/nsi/nsi.c | 1 + 1 file changed, 1 insertion(+) diff --git a/dlls/nsi/nsi.c b/dlls/nsi/nsi.c index 345dce6dfe6..6d8ae40a5c7 100644 --- a/dlls/nsi/nsi.c +++ b/dlls/nsi/nsi.c @@ -70,6 +70,7 @@ DWORD WINAPI NsiAllocateAndGetTable( DWORD unk, const NPI_MODULEID *module, DWOR memset( data, 0, sizeof(data) ); err = NsiEnumerateObjectsAllParameters( unk, 0, module, table, NULL, 0, NULL, 0, NULL, 0, NULL, 0, &num ); if (err) return err; + err = ERROR_OUTOFMEMORY; /* fail if this is the last attempt */ } if (!err) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1978
From: Francois Gouget <fgouget(a)codeweavers.com> The table size typically keeps increasing such that in some cases the size we got is already too small by the time we try to get the table content in the next iteration. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=54328 --- This issue regularly causes NsiAllocateAndGetTable() to fail on my Linux box, maybe because there is always some network traffic. It would go like this: num=64 -> too small, num=264 -> too small, num=266 -> too small, num=267 -> too small, eventualy wasting all 5 attempts (and causing a crash when NsiAllocateAndGetTable() returned success despite it all). --- dlls/nsi/nsi.c | 1 + 1 file changed, 1 insertion(+) diff --git a/dlls/nsi/nsi.c b/dlls/nsi/nsi.c index 6d8ae40a5c7..3f324ef555b 100644 --- a/dlls/nsi/nsi.c +++ b/dlls/nsi/nsi.c @@ -71,6 +71,7 @@ DWORD WINAPI NsiAllocateAndGetTable( DWORD unk, const NPI_MODULEID *module, DWOR err = NsiEnumerateObjectsAllParameters( unk, 0, module, table, NULL, 0, NULL, 0, NULL, 0, NULL, 0, &num ); if (err) return err; err = ERROR_OUTOFMEMORY; /* fail if this is the last attempt */ + num += num >> 4; /* the tables may grow before the next iteration; get ahead */ } if (!err) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1978
One thing to note is that when I only apply the first commit, NsiAllocateAndGetTable() sometimes fails to allocate the tables. This results in both the NsiAllocateAndGetTable() and GetExtendedTcpTable() calls to fail in test_tcp_tables() which is quickly followed by a crash: nsi.c:872: Test failed: AF_INET: 8: got 14 /* ERROR_OUTOFMEMORY */ nsi.c:881: Test failed: AF_INET: 8: got 122 /* ERROR_INSUFFICIENT_BUFFER */ wine: Unhandled page fault on read access to 0116B1F4 at address 00401336 (thread 0024), starting debugger... [...] Backtrace: =>0 0x00401336 test_tcp_tables+0x317(family=<internal error>, table_type=<internal error>) [Z:\home\fgouget\wine\wine-gitlab\dlls\nsi\tests\nsi .c:901] in nsi_test (0x0064fdb8) That makes sense since dyn_tbl was left uninitialized by NsiAllocateAndGetTable(). So one could say that the test is buggy and should take this case into account. Or one can consider that it is up to NsiAllocateAndGetTable() to not fail to allocate the table over trivial issues, which seems to be the whole point of the 5 attempts loop. So that's why I also included the second patch which completely eliminates these failures on my box. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1978#note_21140
This merge request was approved by Huw Davies. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1978
participants (3)
-
Francois Gouget -
Francois Gouget (＠fgouget)
-
Huw Davies (＠huw)