[PATCH 0/2] MR1978: nsi: Fix two NsiAllocateAndGetTable() bugs
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 | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/dlls/nsi/nsi.c b/dlls/nsi/nsi.c index 345dce6dfe6..ba1597e13a9 100644 --- a/dlls/nsi/nsi.c +++ b/dlls/nsi/nsi.c @@ -66,10 +66,11 @@ DWORD WINAPI NsiAllocateAndGetTable( DWORD unk, const NPI_MODULEID *module, DWOR data[2], sizes[2], data[3], sizes[3], &num ); if (err != ERROR_MORE_DATA) break; TRACE( "Short buffer, attempt %d.\n", attempt ); + err = NsiEnumerateObjectsAllParameters( unk, 0, module, table, NULL, 0, NULL, 0, NULL, 0, NULL, 0, &num ); + if (err) goto err; NsiFreeTable( data[0], data[1], data[2], data[3] ); 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) @@ -79,10 +80,11 @@ DWORD WINAPI NsiAllocateAndGetTable( DWORD unk, const NPI_MODULEID *module, DWOR if (sizes[2]) *dynamic_data = data[2]; if (sizes[3]) *static_data = data[3]; *count = num; + return NO_ERROR; } err: - if (err) NsiFreeTable( data[0], data[1], data[2], data[3] ); + NsiFreeTable( data[0], data[1], data[2], data[3] ); return 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 ba1597e13a9..815c942eac9 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 NsiFreeTable( data[0], data[1], data[2], data[3] ); memset( data, 0, sizeof(data) ); err = ERROR_OUTOFMEMORY; /* fail if this is the last attempt */ + num += num >> 4; /* num typically keeps increasing, get ahead */ } if (!err) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1978
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=128493 Your paranoid android. === debian11 (32 bit report) === wmvcore: wmvcore.c:529: Test failed: got wrong thread
Huw Davies (@huw) commented about dlls/nsi/nsi.c:
data[2], sizes[2], data[3], sizes[3], &num ); if (err != ERROR_MORE_DATA) break; TRACE( "Short buffer, attempt %d.\n", attempt ); + err = NsiEnumerateObjectsAllParameters( unk, 0, module, table, NULL, 0, NULL, 0, NULL, 0, NULL, 0, &num ); + if (err) goto err; NsiFreeTable( data[0], data[1], data[2], data[3] ); 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 */
I'd prefer to keep this patch simpler by just adding ```c err = ERROR_OUTOFMEMORY; ``` to the end of the for loop body. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1978#note_21116
Huw Davies (@huw) commented about dlls/nsi/nsi.c:
NsiFreeTable( data[0], data[1], data[2], data[3] ); memset( data, 0, sizeof(data) ); err = ERROR_OUTOFMEMORY; /* fail if this is the last attempt */ + num += num >> 4; /* num typically keeps increasing, get ahead */
While this is fine, the word "typically" both in this comment and in the commit message could do with some qualification. The table may typically grow for TCP connections (although even that is likely only true at system start time), other tables may or may not grow. Perhaps using something like "may grow" instead? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1978#note_21117
On Wed Jan 18 07:57:22 2023 +0000, Huw Davies wrote:
While this is fine, the word "typically" both in this comment and in the commit message could do with some qualification. The table may typically grow for TCP connections (although even that is likely only true at system start time), other tables may or may not grow. Perhaps using something like "may grow" instead? I used the word "typically" because this is quite reproducible on my machine, not just at system start time (which in my case was 62 days ago).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1978#note_21119
On Wed Jan 18 07:53:36 2023 +0000, Huw Davies wrote:
I'd prefer to keep this patch simpler by just adding ```c err = ERROR_OUTOFMEMORY; ``` to the end of the for loop body. Okay. But I think that function has too many ways to deal with errors: 'goto err' in one case, 'break' in another, and 'return err' in yet another.
Also the fact that one can go through the 'err:' block both in case or error and in case of success is not really clean, particularly because the tables may or may not have been freed beforehand. But I guess that can go in a separate patch. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1978#note_21120
participants (4)
-
Francois Gouget -
Francois Gouget (＠fgouget)
-
Huw Davies (＠huw)
-
Marvin