[PATCH v2 0/2] MR4008: secur32: Fix schannel handles.
Access to handle table is actually concurrent (and races / corrupts memory / crashes in practice when apps use different security contexts from different threads). Then, unrelated to races, schan_handle_table can be reallocated and then schan_free_handles chain points to old memory location. Alternatively to patch 2, that could be fixed up on reallocating schan_handle_table, but it seems to me we can as well just scan the handle table for free slots. -- v2: secur32: Get rid of schannel free handle list. https://gitlab.winehq.org/wine/wine/-/merge_requests/4008
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/secur32/schannel.c | 47 +++++++++++++++++++++++++++++------------ 1 file changed, 34 insertions(+), 13 deletions(-) diff --git a/dlls/secur32/schannel.c b/dlls/secur32/schannel.c index d16692a4c3c..d2e4a07fa23 100644 --- a/dlls/secur32/schannel.c +++ b/dlls/secur32/schannel.c @@ -71,6 +71,7 @@ static struct schan_handle *schan_handle_table; static struct schan_handle *schan_free_handles; static SIZE_T schan_handle_table_size; static SIZE_T schan_handle_count; +static SRWLOCK handle_table_lock = SRWLOCK_INIT; /* Protocols enabled, only those may be used for the connection. */ static DWORD config_enabled_protocols; @@ -81,22 +82,24 @@ static DWORD config_default_disabled_protocols; static ULONG_PTR schan_alloc_handle(void *object, enum schan_handle_type type) { struct schan_handle *handle; + ULONG_PTR index = SCHAN_INVALID_HANDLE; + AcquireSRWLockExclusive(&handle_table_lock); if (schan_free_handles) { - DWORD index = schan_free_handles - schan_handle_table; /* Use a free handle */ handle = schan_free_handles; if (handle->type != SCHAN_HANDLE_FREE) { - ERR("Handle %ld(%p) is in the free list, but has type %#x.\n", index, handle, handle->type); - return SCHAN_INVALID_HANDLE; + ERR("Handle %p is in the free list, but has type %#x.\n", handle, handle->type); + goto done; } + index = schan_free_handles - schan_handle_table; schan_free_handles = handle->object; handle->object = object; handle->type = type; - return index; + goto done; } if (!(schan_handle_count < schan_handle_table_size)) { @@ -106,7 +109,7 @@ static ULONG_PTR schan_alloc_handle(void *object, enum schan_handle_type type) if (!new_table) { ERR("Failed to grow the handle table\n"); - return SCHAN_INVALID_HANDLE; + goto done; } schan_handle_table = new_table; schan_handle_table_size = new_size; @@ -116,21 +119,30 @@ static ULONG_PTR schan_alloc_handle(void *object, enum schan_handle_type type) handle->object = object; handle->type = type; - return handle - schan_handle_table; + index = handle - schan_handle_table; + +done: + ReleaseSRWLockExclusive(&handle_table_lock); + return index; } static void *schan_free_handle(ULONG_PTR handle_idx, enum schan_handle_type type) { struct schan_handle *handle; - void *object; + void *object = NULL; if (handle_idx == SCHAN_INVALID_HANDLE) return NULL; - if (handle_idx >= schan_handle_count) return NULL; + + AcquireSRWLockExclusive(&handle_table_lock); + + if (handle_idx >= schan_handle_count) + goto done; + handle = &schan_handle_table[handle_idx]; if (handle->type != type) { ERR("Handle %Id(%p) is not of type %#x\n", handle_idx, handle, type); - return NULL; + goto done; } object = handle->object; @@ -138,23 +150,32 @@ static void *schan_free_handle(ULONG_PTR handle_idx, enum schan_handle_type type handle->type = SCHAN_HANDLE_FREE; schan_free_handles = handle; +done: + ReleaseSRWLockExclusive(&handle_table_lock); return object; } static void *schan_get_object(ULONG_PTR handle_idx, enum schan_handle_type type) { struct schan_handle *handle; + void *object = NULL; if (handle_idx == SCHAN_INVALID_HANDLE) return NULL; - if (handle_idx >= schan_handle_count) return NULL; + + AcquireSRWLockShared(&handle_table_lock); + if (handle_idx >= schan_handle_count) + goto done; handle = &schan_handle_table[handle_idx]; if (handle->type != type) { - ERR("Handle %Id(%p) is not of type %#x\n", handle_idx, handle, type); - return NULL; + ERR("Handle %Id(%p) is not of type %#x (%#x)\n", handle_idx, handle, type, handle->type); + goto done; } + object = handle->object; - return handle->object; +done: + ReleaseSRWLockShared(&handle_table_lock); + return object; } static void read_config(void) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4008
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/secur32/schannel.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/dlls/secur32/schannel.c b/dlls/secur32/schannel.c index d2e4a07fa23..b175fdba4f1 100644 --- a/dlls/secur32/schannel.c +++ b/dlls/secur32/schannel.c @@ -68,7 +68,6 @@ struct schan_context }; static struct schan_handle *schan_handle_table; -static struct schan_handle *schan_free_handles; static SIZE_T schan_handle_table_size; static SIZE_T schan_handle_count; static SRWLOCK handle_table_lock = SRWLOCK_INIT; @@ -82,23 +81,17 @@ static DWORD config_default_disabled_protocols; static ULONG_PTR schan_alloc_handle(void *object, enum schan_handle_type type) { struct schan_handle *handle; - ULONG_PTR index = SCHAN_INVALID_HANDLE; + ULONG_PTR index = SCHAN_INVALID_HANDLE, i; AcquireSRWLockExclusive(&handle_table_lock); - if (schan_free_handles) + for (i = 0; i < schan_handle_table_size; ++i) { - /* Use a free handle */ - handle = schan_free_handles; + handle = &schan_handle_table[i]; if (handle->type != SCHAN_HANDLE_FREE) - { - ERR("Handle %p is in the free list, but has type %#x.\n", handle, handle->type); - goto done; - } - index = schan_free_handles - schan_handle_table; - schan_free_handles = handle->object; + continue; handle->object = object; handle->type = type; - + index = i; goto done; } if (!(schan_handle_count < schan_handle_table_size)) @@ -146,9 +139,8 @@ static void *schan_free_handle(ULONG_PTR handle_idx, enum schan_handle_type type } object = handle->object; - handle->object = schan_free_handles; + handle->object = NULL; handle->type = SCHAN_HANDLE_FREE; - schan_free_handles = handle; done: ReleaseSRWLockExclusive(&handle_table_lock); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4008
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=138276 Your paranoid android. === debian11b (64 bit WoW report) === urlmon: url.c:3988: Test failed: request failed: 12157 url.c:4104: Test failed: Skipping https tests
This merge request was approved by Hans Leidekker. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4008
participants (4)
-
Hans Leidekker (＠hans) -
Marvin -
Paul Gofman
-
Paul Gofman (＠gofman)