Signed-off-by: Zebediah Figura z.figura12@gmail.com --- 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().
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) {
Signed-off-by: Zebediah Figura z.figura12@gmail.com --- Avoids a potential though unlikely race whereby an asynchronous custom action receives the wrong thread handle.
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: Zebediah Figura z.figura12@gmail.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().
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: Zebediah Figura z.figura12@gmail.com --- dlls/msi/tests/custom.c | 15 +++++++++++++++ dlls/msi/tests/custom.spec | 2 ++ dlls/msi/tests/install.c | 10 +++++++++- 3 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/dlls/msi/tests/custom.c b/dlls/msi/tests/custom.c index 3989ca8..5075861 100644 --- a/dlls/msi/tests/custom.c +++ b/dlls/msi/tests/custom.c @@ -1169,6 +1169,21 @@ UINT WINAPI process2(MSIHANDLE hinst) return ERROR_SUCCESS; }
+UINT WINAPI async1(MSIHANDLE hinst) +{ + HANDLE event = CreateEventA(NULL, TRUE, FALSE, "wine_msi_async_test"); + DWORD r = WaitForSingleObject(event, 10000); + ok(hinst, !r, "wait timed out\n"); + return ERROR_SUCCESS; +} + +UINT WINAPI async2(MSIHANDLE hinst) +{ + HANDLE event = CreateEventA(NULL, TRUE, FALSE, "wine_msi_async_test"); + SetEvent(event); + 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");
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=40736
Your paranoid android.
=== w7u (32 bit install) === The test timed out
=== w7u (32 bit msi) === The test timed out
=== w1064 (64 bit record) === TestBot process got stuck or died unexpectedly The previous 1 run(s) terminated abnormally
=== w2008s64 (64 bit source) === TestBot process got stuck or died unexpectedly The previous 1 run(s) terminated abnormally
On Wed, 2018-08-15 at 19:47 -0500, Zebediah Figura wrote:
Signed-off-by: Zebediah Figura z.figura12@gmail.com
dlls/msi/tests/custom.c | 15 +++++++++++++++ dlls/msi/tests/custom.spec | 2 ++ dlls/msi/tests/install.c | 10 +++++++++- 3 files changed, 26 insertions(+), 1 deletion(-)
This fails for me some of the time:
0009:err:msi:ITERATE_Actions Execution halted, action L"testretval" returned 1602 0009:err:msi:custom_get_thread_return Invalid Return Code 1 0009:err:msi:ITERATE_Actions Execution halted, action L"testretval" returned 1603 custom.c:1176: Test failed: wait timed out custom.c:1176: Test failed: wait timed out
On 16/08/18 06:30, Hans Leidekker wrote:
On Wed, 2018-08-15 at 19:47 -0500, Zebediah Figura wrote:
Signed-off-by: Zebediah Figura z.figura12@gmail.com
dlls/msi/tests/custom.c | 15 +++++++++++++++ dlls/msi/tests/custom.spec | 2 ++ dlls/msi/tests/install.c | 10 +++++++++- 3 files changed, 26 insertions(+), 1 deletion(-)
This fails for me some of the time:
0009:err:msi:ITERATE_Actions Execution halted, action L"testretval" returned 1602 0009:err:msi:custom_get_thread_return Invalid Return Code 1 0009:err:msi:ITERATE_Actions Execution halted, action L"testretval" returned 1603 custom.c:1176: Test failed: wait timed out custom.c:1176: Test failed: wait timed out
Thanks, I see the problem. I've resent the series with a fix for this.
-
Hans Leidekker -
Marvin -
Zebediah Figura