May 2023 - Wine-GitLab - list.winehq.org

Re: [PATCH v2 0/1] MR2827: ntdll: Correctly identify hidden files
by David Kahurani (＠kahurani) May 16, 2023

May 16, 2023

On Tue May 16 14:05:22 2023 +0000, **** wrote: > Marvin replied on the mailing list: > ``` > 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 full results can be found at: > https://testbot.winehq.org/JobDetails.pl?Key=132727 > Your paranoid android. > === debian11 (build log) === > error: corrupt patch at line 38 > Task: Patch failed to apply > === debian11b (build log) === > error: corrupt patch at line 38 > Task: Patch failed to apply > ``` Interesting indeed, the issue is gone after adjusting the patch to fix the issue with directories. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2827#note_32894

1 0

Re: [PATCH v2 0/1] MR2827: ntdll: Correctly identify hidden files
by Sven Baars (＠sbaars) May 16, 2023

May 16, 2023

On Tue May 16 13:04:00 2023 +0000, David Kahurani wrote: > Yeah, I noticed that my changes alienate directories which should be > part of the "dot files" but I think I have come across a different > problem that might be harder to solve. > The tests don't seem to like these changes.... > ``` > GetTempPathA( MAX_PATH, temppath ); > GetTempFileNameA( temppath, ".foo", 0, filename ); > h = CreateFileA( filename, GENERIC_READ | GENERIC_WRITE, 0, NULL, > CREATE_ALWAYS, FILE_FLAG_DELETE_ON_CLOSE, 0 ); > ok( h != INVALID_HANDLE_VALUE, "failed to create temp file\n" ); > status = nt_get_file_attrs(filename, &attrs); > ok( status == STATUS_SUCCESS, "got %#lx\n", status ); > ok( !(attrs & FILE_ATTRIBUTE_HIDDEN), "got attributes %#lx\n", attrs ); > ``` > This test creates a temporary file(hidden as per its unix path but of > course not hidden as per Windows functioning). The test expects that the > file is not hidden but our changes make it so that the file is reported > as hidden. > I suspect the old code was carefully crafted so as to avoid marking a > file as hidden unless it is absolutely necessary. If I just do this, I don't seem to get any additional failures. We should add a test demonstrating the current problem though. ``` diff --git a/dlls/ntdll/unix/file.c b/dlls/ntdll/unix/file.c index 0263d53716b..a740408fc58 100644 --- a/dlls/ntdll/unix/file.c +++ b/dlls/ntdll/unix/file.c @@ -1305,8 +1305,7 @@ static BOOL is_hidden_file( const char *name ) while (p > name && p[-1] != '/') p--; if (*p++ != '.') return FALSE; if (!*p || *p == '/') return FALSE; /* "." directory */ - if (*p++ != '.') return FALSE; - if (!*p || *p == '/') return FALSE; /* ".." directory */ + if (*p++ == '.' && (!*p || *p == '/')) return FALSE; /* ".." directory */ return TRUE; } ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2827#note_32892

2 1

[PATCH v3 0/2] MR2752: combase: Prevent use-after-free due to a race with proxy_manager_destroy.
by Jinoh Kang (＠iamahuman) May 16, 2023

May 16, 2023

Today, find_proxy_manager() may return a proxy manager that is about to be freed. This happens when the proxy manager's reference count reaches zero, but proxy_manager_destroy() has not removed the proxy manager from the apartment proxy list. Fix this by only incrementing the reference count if it was nonzero. If the reference count is zero, proxy_manager_destroy() will proceed to destroy the proxy manager after the current thread releases the apartment lock (apt->cs). Granted, this is fragile and far from elegant, but it seems like a safe approach for the time being. Also, the critical section and "safe" refcount increment will prevent the following race condition scenario: > 1. Thread X: A proxy manager's refcount reaches 0 and is about to be released. > 2. Thread Y tries to grab it, calls AddRef, notices it returns 1, and drops it. > 3. Thread Z tries to grab it, calls AddRef, notices it returns 2, and returns it. > 4. Thread X then proceeds to remove it from the list and free the object. -- v3: sombase: Prevent use-after-free due to a race with proxy_manager_destroy. https://gitlab.winehq.org/wine/wine/-/merge_requests/2752

3 3

[PATCH v2 0/2] MR2752: combase: Prevent use-after-free due to a race with proxy_manager_destroy.
by Jinoh Kang (＠iamahuman) May 16, 2023

May 16, 2023

Today, find_proxy_manager() may return a proxy manager that is about to be freed. This happens when the proxy manager's reference count reaches zero, but proxy_manager_destroy() has not removed the proxy manager from the apartment proxy list. Fix this by only incrementing the reference count if it was nonzero. If the reference count is zero, proxy_manager_destroy() will proceed to destroy the proxy manager after the current thread releases the apartment lock (apt->cs). Granted, this is fragile and far from elegant, but it seems like a safe approach for the time being. Also, the critical section and "safe" refcount increment will prevent the following race condition scenario: > 1. Thread X: A proxy manager's refcount reaches 0 and is about to be released. > 2. Thread Y tries to grab it, calls AddRef, notices it returns 1, and drops it. > 3. Thread Z tries to grab it, calls AddRef, notices it returns 2, and returns it. > 4. Thread X then proceeds to remove it from the list and free the object. -- v2: combase: Prevent use-after-free due to a race with proxy_manager_destroy. https://gitlab.winehq.org/wine/wine/-/merge_requests/2752

3 4

Re: [PATCH v2 0/1] MR2827: ntdll: Correctly identify hidden files
by David Kahurani (＠kahurani) May 16, 2023

May 16, 2023

On Tue May 16 12:34:25 2023 +0000, Sven Baars wrote: > This line and the following line should probably be combined into one to > fix the issue. Yeah, I noticed that my changes alienate directories which should be part of the "dot files" but I think I have come across a different problem that might be harder to solve. The tests don't seem to like these changes.... ``` GetTempPathA( MAX_PATH, temppath ); GetTempFileNameA( temppath, ".foo", 0, filename ); h = CreateFileA( filename, GENERIC_READ | GENERIC_WRITE, 0, NULL, CREATE_ALWAYS, FILE_FLAG_DELETE_ON_CLOSE, 0 ); ok( h != INVALID_HANDLE_VALUE, "failed to create temp file\n" ); status = nt_get_file_attrs(filename, &attrs); ok( status == STATUS_SUCCESS, "got %#lx\n", status ); ok( !(attrs & FILE_ATTRIBUTE_HIDDEN), "got attributes %#lx\n", attrs ); ``` This test creates a temporary file(hidden as per its unix path but of course not hidden as per Windows functioning). The test expects that the file is not hidden but our changes make it so that the file is reported as hidden. I suspect the old code was carefully crafted so as to avoid marking a file as hidden unless it is absolutely necessary. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2827#note_32883

1 0

Re: [PATCH v2 0/1] MR2827: ntdll: Correctly identify hidden files
by Sven Baars (＠sbaars) May 16, 2023

May 16, 2023

Sven Baars (@sbaars) commented about dlls/ntdll/unix/file.c: > * Check if the specified file should be hidden based on its unix path and the show dot files option. > */ > -static BOOL is_hidden_file( const char *name ) > +static BOOL is_hidden_file( const char *path ) > { > const char *p; > + struct stat st; > > if (show_dot_files) return FALSE; > > - p = name + strlen( name ); > - while (p > name && p[-1] == '/') p--; > - while (p > name && p[-1] != '/') p--; > - if (*p++ != '.') return FALSE; > - if (!*p || *p == '/') return FALSE; /* "." directory */ > - if (*p++ != '.') return FALSE; This line and the following line should probably be combined into one to fix the issue. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2827#note_32881

1 0

[PATCH 0/1] MR2829: dinput/tests: Add a zero-terminator for hardware ids (Valgrind).
by Rémi Bernon May 16, 2023

May 16, 2023

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2829

1 1

[PATCH 0/2] MR2752: combase: Prevent use-after-free due to a race with proxy_manager_destroy.
by Jinoh Kang (＠iamahuman) May 16, 2023

May 16, 2023

Today, find_proxy_manager() may return a proxy manager that is about to be freed. This happens when the proxy manager's reference count reaches zero, but proxy_manager_destroy() has not removed the proxy manager from the apartment proxy list. Fix this by only incrementing the reference count if it was nonzero. If the reference count is zero, proxy_manager_destroy() will proceed to destroy the proxy manager after the current thread releases the apartment lock (apt->cs). Granted, this is fragile is far from elegant, but it seems like a safe approach for the time being. Also, the critical section and "safe" refcount increment will prevent the following race condition scenario: > 1. Thread X: A proxy manager's refcount reaches 0 and is about to be released. > 2. Thread Y tries to grab it, calls AddRef, notices it returns 1, and drops it. > 3. Thread Z tries to grab it, calls AddRef, notices it returns 2, and returns it. > 4. Thread X then proceeds to remove it from the list and free the object. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2752

4 6

[PATCH 0/4] MR2824: wine{alsa,coreaudio,oss,pulse}: Move AudioRenderClient into mmdevapi.
by Davide Beatrici (＠davidebeatrici) May 16, 2023

May 16, 2023

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2824

3 5

Re: [PATCH v2 0/1] MR2827: ntdll: Correctly identify hidden files
by David Kahurani (＠kahurani) May 16, 2023

May 16, 2023

On Tue May 16 07:36:20 2023 +0000, **** wrote: > Marvin replied on the mailing list: > ``` > 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=132716 > Your paranoid android. > === debian11 (32 bit report) === > ntdll: > file.c:4097: Test failed: got attributes 0x22 > === debian11 (32 bit zh:CN report) === > ntdll: > file.c:4097: Test failed: got attributes 0x22 > === debian11b (64 bit WoW report) === > ntdll: > file.c:4097: Test failed: got attributes 0x22 > ``` It looks to me like the only way around this would be to treat prefixes separately from other paths but it is not clear to me how one would positively identify a path that lies within a prefix. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2827#note_32872

1 0