[PATCH v2 1/5] msi: Don't start the custom action server inside of custom_client_thread().
Asynchronous custom actions don't wait for custom_client_thread() to terminate, so it's possible for two consecutive actions to both try to start the server at once, causing failures when the second action e.g. receives ERROR_PIPE_BUSY from CreateNamedPipe().
Signed-off-by: Zebediah Figura z.figura12@gmail.com --- For bug 45483.
dlls/msi/custom.c | 61 ++++++++++++++++++++++++++++--------------------------- 1 file changed, 31 insertions(+), 30 deletions(-)
diff --git a/dlls/msi/custom.c b/dlls/msi/custom.c index b250b1b..a9bb81f 100644 --- a/dlls/msi/custom.c +++ b/dlls/msi/custom.c @@ -388,6 +388,7 @@ typedef struct _msi_custom_action_info { LPWSTR action; INT type; GUID guid; + DWORD arch; } msi_custom_action_info;
static void release_custom_action_data( msi_custom_action_info *info ) @@ -662,45 +663,16 @@ void custom_stop_server(HANDLE process, HANDLE pipe) static DWORD WINAPI custom_client_thread(void *arg) { msi_custom_action_info *info = arg; - RPC_STATUS status; DWORD64 thread64; HANDLE process; HANDLE thread; HANDLE pipe; - DWORD arch; DWORD size; - BOOL ret; UINT rc;
CoInitializeEx(NULL, COINIT_MULTITHREADED); /* needed to marshal streams */
- if (!info->package->rpc_server_started) - { - status = RpcServerUseProtseqEpW(ncalrpcW, RPC_C_PROTSEQ_MAX_REQS_DEFAULT, - endpoint_lrpcW, NULL); - if (status != RPC_S_OK) - { - ERR("RpcServerUseProtseqEp failed: %#x\n", status); - return status; - } - - status = RpcServerRegisterIfEx(s_IWineMsiRemote_v0_0_s_ifspec, NULL, NULL, - RPC_IF_AUTOLISTEN, RPC_C_LISTEN_MAX_CALLS_DEFAULT, NULL); - if (status != RPC_S_OK) - { - ERR("RpcServerRegisterIfEx failed: %#x\n", status); - return status; - } - - info->package->rpc_server_started = 1; - } - - ret = GetBinaryTypeW(info->source, &arch); - if (!ret) - arch = (sizeof(void *) == 8 ? SCS_64BIT_BINARY : SCS_32BIT_BINARY); - - custom_start_server(info->package, arch); - if (arch == SCS_32BIT_BINARY) + if (info->arch == SCS_32BIT_BINARY) { process = info->package->custom_server_32_process; pipe = info->package->custom_server_32_pipe; @@ -741,6 +713,8 @@ static msi_custom_action_info *do_msidbCustomActionTypeDll( MSIPACKAGE *package, INT type, LPCWSTR source, LPCWSTR target, LPCWSTR action ) { msi_custom_action_info *info; + RPC_STATUS status; + BOOL ret;
info = msi_alloc( sizeof *info ); if (!info) @@ -759,6 +733,33 @@ static msi_custom_action_info *do_msidbCustomActionTypeDll( list_add_tail( &msi_pending_custom_actions, &info->entry ); LeaveCriticalSection( &msi_custom_action_cs );
+ if (!package->rpc_server_started) + { + status = RpcServerUseProtseqEpW(ncalrpcW, RPC_C_PROTSEQ_MAX_REQS_DEFAULT, + endpoint_lrpcW, NULL); + if (status != RPC_S_OK) + { + ERR("RpcServerUseProtseqEp failed: %#x\n", status); + return NULL; + } + + status = RpcServerRegisterIfEx(s_IWineMsiRemote_v0_0_s_ifspec, NULL, NULL, + RPC_IF_AUTOLISTEN, RPC_C_LISTEN_MAX_CALLS_DEFAULT, NULL); + if (status != RPC_S_OK) + { + ERR("RpcServerRegisterIfEx failed: %#x\n", status); + return NULL; + } + + info->package->rpc_server_started = 1; + } + + ret = GetBinaryTypeW(source, &info->arch); + if (!ret) + info->arch = (sizeof(void *) == 8 ? SCS_64BIT_BINARY : SCS_32BIT_BINARY); + + custom_start_server(package, info->arch); + info->handle = CreateThread(NULL, 0, custom_client_thread, info, 0, NULL); if (!info->handle) {
Avoids a potential race whereby an asynchronous custom action receives the wrong thread handle.
Signed-off-by: Zebediah Figura z.figura12@gmail.com --- dlls/msi/custom.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/dlls/msi/custom.c b/dlls/msi/custom.c index a9bb81f..9418b42 100644 --- a/dlls/msi/custom.c +++ b/dlls/msi/custom.c @@ -683,6 +683,8 @@ static DWORD WINAPI custom_client_thread(void *arg) pipe = info->package->custom_server_64_pipe; }
+ EnterCriticalSection(&msi_custom_action_cs); + if (!WriteFile(pipe, &info->guid, sizeof(info->guid), &size, NULL) || size != sizeof(info->guid)) { @@ -695,6 +697,8 @@ static DWORD WINAPI custom_client_thread(void *arg) return GetLastError(); }
+ LeaveCriticalSection(&msi_custom_action_cs); + if (DuplicateHandle(process, (HANDLE)(DWORD_PTR)thread64, GetCurrentProcess(), &thread, 0, FALSE, DUPLICATE_SAME_ACCESS | DUPLICATE_CLOSE_SOURCE)) {
Signed-off-by: Hans Leidekker hans@codeweavers.com
This is unnecessary, and may have always been so. The struct will either be freed after it completes synchronously, or after it has completed asynchronously in ACTION_FinishCustomActions().
Signed-off-by: Zebediah Figura z.figura12@gmail.com --- dlls/msi/custom.c | 46 ++++++++++++++-------------------------------- 1 file changed, 14 insertions(+), 32 deletions(-)
diff --git a/dlls/msi/custom.c b/dlls/msi/custom.c index 9418b42..950681e 100644 --- a/dlls/msi/custom.c +++ b/dlls/msi/custom.c @@ -380,7 +380,6 @@ static UINT wait_process_handle(MSIPACKAGE* package, UINT type,
typedef struct _msi_custom_action_info { struct list entry; - LONG refs; MSIPACKAGE *package; LPWSTR source; LPWSTR target; @@ -391,31 +390,22 @@ typedef struct _msi_custom_action_info { DWORD arch; } msi_custom_action_info;
-static void release_custom_action_data( msi_custom_action_info *info ) +static void free_custom_action_data( msi_custom_action_info *info ) { EnterCriticalSection( &msi_custom_action_cs );
- if (!--info->refs) - { - list_remove( &info->entry ); - if (info->handle) - CloseHandle( info->handle ); - msi_free( info->action ); - msi_free( info->source ); - msi_free( info->target ); - msiobj_release( &info->package->hdr ); - msi_free( info ); - } + list_remove( &info->entry ); + if (info->handle) + CloseHandle( info->handle ); + msi_free( info->action ); + msi_free( info->source ); + msi_free( info->target ); + msiobj_release( &info->package->hdr ); + msi_free( info );
LeaveCriticalSection( &msi_custom_action_cs ); }
-/* must be called inside msi_custom_action_cs if info is in the pending custom actions list */ -static void addref_custom_action_data( msi_custom_action_info *info ) -{ - info->refs++; - } - static UINT wait_thread_handle( msi_custom_action_info *info ) { UINT rc = ERROR_SUCCESS; @@ -429,7 +419,7 @@ static UINT wait_thread_handle( msi_custom_action_info *info ) if (!(info->type & msidbCustomActionTypeContinue)) rc = custom_get_thread_return( info->package, info->handle );
- release_custom_action_data( info ); + free_custom_action_data( info ); } else { @@ -450,7 +440,6 @@ static msi_custom_action_info *find_action_by_guid( const GUID *guid ) { if (IsEqualGUID( &info->guid, guid )) { - addref_custom_action_data( info ); found = TRUE; break; } @@ -725,7 +714,6 @@ static msi_custom_action_info *do_msidbCustomActionTypeDll( return NULL;
msiobj_addref( &package->hdr ); - info->refs = 2; /* 1 for our caller and 1 for thread we created */ info->package = package; info->type = type; info->target = strdupW( target ); @@ -767,9 +755,7 @@ static msi_custom_action_info *do_msidbCustomActionTypeDll( info->handle = CreateThread(NULL, 0, custom_client_thread, info, 0, NULL); if (!info->handle) { - /* release both references */ - release_custom_action_data( info ); - release_custom_action_data( info ); + free_custom_action_data( info ); return NULL; }
@@ -1056,7 +1042,6 @@ static DWORD ACTION_CallScript( const GUID *guid ) else ERR("failed to create handle for %p\n", info->package );
- release_custom_action_data( info ); return r; }
@@ -1085,7 +1070,6 @@ static msi_custom_action_info *do_msidbCustomActionTypeScript( return NULL;
msiobj_addref( &package->hdr ); - info->refs = 2; /* 1 for our caller and 1 for thread we created */ info->package = package; info->type = type; info->target = strdupW( function ); @@ -1100,9 +1084,7 @@ static msi_custom_action_info *do_msidbCustomActionTypeScript( info->handle = CreateThread( NULL, 0, ScriptThread, &info->guid, 0, NULL ); if (!info->handle) { - /* release both references */ - release_custom_action_data( info ); - release_custom_action_data( info ); + free_custom_action_data( info ); return NULL; }
@@ -1496,7 +1478,8 @@ void ACTION_FinishCustomActions(const MSIPACKAGE* package) EnterCriticalSection( &msi_custom_action_cs ); LIST_FOR_EACH_ENTRY_SAFE( info, cursor, &msi_pending_custom_actions, msi_custom_action_info, entry ) { - if (info->package == package) release_custom_action_data( info ); + if (info->package == package) + free_custom_action_data( info ); } LeaveCriticalSection( &msi_custom_action_cs ); } @@ -1514,6 +1497,5 @@ UINT __cdecl s_remote_GetActionInfo(const GUID *guid, int *type, LPWSTR *dll, LP *dll = strdupW(info->source); *func = strdupWtoA(info->target);
- release_custom_action_data(info); return ERROR_SUCCESS; }
Signed-off-by: Hans Leidekker hans@codeweavers.com
Since it could potentially change on us during an asynchronous custom action.
Signed-off-by: Zebediah Figura z.figura12@gmail.com --- programs/msiexec/msiexec.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/programs/msiexec/msiexec.c b/programs/msiexec/msiexec.c index ead3bfb..efbcb5c 100644 --- a/programs/msiexec/msiexec.c +++ b/programs/msiexec/msiexec.c @@ -28,6 +28,7 @@ #include <stdio.h>
#include "wine/debug.h" +#include "wine/heap.h" #include "wine/unicode.h"
#include "initguid.h" @@ -400,20 +401,21 @@ extern UINT CDECL __wine_msi_call_dll_function(GUID *guid);
static DWORD CALLBACK custom_action_thread(void *arg) { - GUID *guid = arg; - return __wine_msi_call_dll_function(guid); + GUID guid = *(GUID *)arg; + heap_free(arg); + return __wine_msi_call_dll_function(&guid); }
static int custom_action_server(const WCHAR *arg) { static const WCHAR pipe_name[] = {'\','\','.','\','p','i','p','e','\','m','s','i','c','a','_','%','x','_','%','d',0}; DWORD client_pid = atoiW(arg); + GUID guid, *thread_guid; DWORD64 thread64; WCHAR buffer[24]; HANDLE thread; HANDLE pipe; DWORD size; - GUID guid;
TRACE("%s\n", debugstr_w(arg));
@@ -443,7 +445,9 @@ static int custom_action_server(const WCHAR *arg) return 0; }
- thread = CreateThread(NULL, 0, custom_action_thread, &guid, 0, NULL); + thread_guid = heap_alloc(sizeof(GUID)); + memcpy(thread_guid, &guid, sizeof(GUID)); + thread = CreateThread(NULL, 0, custom_action_thread, thread_guid, 0, NULL);
/* give the thread handle to the client to wait on, since we might have * to run a nested action and can't block during this one */
Signed-off-by: Hans Leidekker hans@codeweavers.com
Signed-off-by: Zebediah Figura z.figura12@gmail.com --- v2: improve tests to ensure that both actions run
dlls/msi/tests/custom.c | 21 +++++++++++++++++++++ dlls/msi/tests/custom.spec | 2 ++ dlls/msi/tests/install.c | 10 +++++++++- 3 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/dlls/msi/tests/custom.c b/dlls/msi/tests/custom.c index 3989ca8..6332d68 100644 --- a/dlls/msi/tests/custom.c +++ b/dlls/msi/tests/custom.c @@ -1169,6 +1169,27 @@ UINT WINAPI process2(MSIHANDLE hinst) return ERROR_SUCCESS; }
+UINT WINAPI async1(MSIHANDLE hinst) +{ + HANDLE event = CreateEventA(NULL, TRUE, FALSE, "wine_msi_async_test"); + HANDLE event2 = CreateEventA(NULL, TRUE, FALSE, "wine_msi_async_test2"); + DWORD r = WaitForSingleObject(event, 10000); + ok(hinst, !r, "wait timed out\n"); + SetEvent(event2); + return ERROR_SUCCESS; +} + +UINT WINAPI async2(MSIHANDLE hinst) +{ + HANDLE event = CreateEventA(NULL, TRUE, FALSE, "wine_msi_async_test"); + HANDLE event2 = CreateEventA(NULL, TRUE, FALSE, "wine_msi_async_test2"); + DWORD r; + SetEvent(event); + r = WaitForSingleObject(event2, 10000); + ok(hinst, !r, "wait timed out\n"); + return ERROR_SUCCESS; +} + static BOOL pf_exists(const char *file) { char path[MAX_PATH]; diff --git a/dlls/msi/tests/custom.spec b/dlls/msi/tests/custom.spec index b7cbe58..5d78377 100644 --- a/dlls/msi/tests/custom.spec +++ b/dlls/msi/tests/custom.spec @@ -5,6 +5,8 @@ @ stdcall nested(long) @ stdcall process1(long) @ stdcall process2(long) +@ stdcall async1(long) +@ stdcall async2(long)
@ stdcall cf_present(long) @ stdcall cf_absent(long) diff --git a/dlls/msi/tests/install.c b/dlls/msi/tests/install.c index e147df6..4fa1862 100644 --- a/dlls/msi/tests/install.c +++ b/dlls/msi/tests/install.c @@ -677,6 +677,8 @@ static const CHAR ca1_install_exec_seq_dat[] = "Action\tCondition\tSequence\n" "process1\tTEST_PROCESS\t720\n" "process2\tTEST_PROCESS\t721\n" "process_deferred\tTEST_PROCESS\t722\n" + "async1\tTEST_ASYNC\t730\n" + "async2\tTEST_ASYNC\t731\n" "InstallFinalize\t\t800\n";
static const CHAR ca1_custom_action_dat[] = "Action\tType\tSource\tTarget\n" @@ -689,6 +691,8 @@ static const CHAR ca1_custom_action_dat[] = "Action\tType\tSource\tTarget\n" "process1\t1\tcustom.dll\tprocess1\n" "process2\t1\tcustom.dll\tprocess2\n" "process_deferred\t1025\tcustom.dll\tprocess2\n" + "async1\t129\tcustom.dll\tasync1\n" + "async2\t1\tcustom.dll\tasync2\n" "testretval\t1\tcustom.dll\ttest_retval\n";
static const CHAR ca1_test_seq_dat[] = "Action\tCondition\tSequence\n" @@ -4129,7 +4133,7 @@ static void test_customaction1(void) r = MsiInstallProductA(msifile, "TEST_RETVAL=0"); ok(r == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got %u\n", r);
- r = MsiInstallProductA(msifile, "TEST_RETVAL=1626"); /* ERROR_FUNCTION_NOT_CALLED*/ + r = MsiInstallProductA(msifile, "TEST_RETVAL=1626"); /* ERROR_FUNCTION_NOT_CALLED */ ok(r == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got %u\n", r);
r = MsiInstallProductA(msifile, "TEST_RETVAL=1602"); @@ -4146,6 +4150,10 @@ static void test_customaction1(void) r = MsiInstallProductA(msifile, "TEST_PROCESS=1"); ok(!r, "got %u\n", r);
+ /* test asynchronous actions (msidbCustomActionTypeAsync) */ + r = MsiInstallProductA(msifile, "TEST_ASYNC=1"); + ok(!r, "got %u\n", r); + delete_test_files(); DeleteFileA(msifile); DeleteFileA("unus");
Signed-off-by: Hans Leidekker hans@codeweavers.com
Hi,
While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=40783
Your paranoid android.
=== wvistau64 (32 bit action) === The test timed out
=== w864 (32 bit db) === db.c:7641: Test failed: Failed to create database db.c:3084: Test failed: Failed to open summaryinfo db.c:3088: Test failed: Failed to set summary info db.c:3092: Test failed: Failed to set summary info db.c:3096: Test failed: Failed to set summary info db.c:3100: Test failed: Failed to set summary info db.c:3104: Test failed: Failed to set summary info db.c:3107: Test failed: Failed to set summary info db.c:3110: Test failed: Failed to set summary info db.c:3113: Test failed: Failed to make summary info persist db.c:240: Test failed: Failed to create Directory table: 6 db.c:228: Test failed: Failed to create CustomAction table: 6 db.c:7648: Test failed: failed to insert into CustomAction table: 6 db.c:7657: Test failed: Expected ERROR_SUCCESS, got 6 db.c:7663: Test failed: Expected ERROR_SUCCESS, got 6 db.c:7664: Test failed: Expected "", got "kiwi" db.c:7665: Test failed: Expected 0, got 260 db.c:7669: Test failed: Expected ERROR_SUCCESS, got 6 db.c:7675: Test failed: Expected ERROR_SUCCESS, got 6 db.c:7676: Test failed: Expected "grape", got "kiwi" db.c:7677: Test failed: Expected 5, got 260 db.c:7683: Test failed: Expected ERROR_SUCCESS, got 6 db.c:7689: Test failed: Expected ERROR_SUCCESS, got 6 db.c:7692: Test failed: Expected "", got "kiwi" db.c:7693: Test failed: Expected 0, got 260
=== w7u (32 bit install) === The test timed out
=== w7u (32 bit msi) === The test timed out The task timed out
-
Hans Leidekker -
Marvin -
Zebediah Figura