[PATCH v2 0/3] MR11017: msi: Catch all exceptions in the custom DLL procedure.
-- v2: msi: Always propagate custom action return value. msi: Return ERROR_INSTALL_FAILURE on an exception in the custom action. msi: Catch all exceptions in the custom DLL procedure. https://gitlab.winehq.org/wine/wine/-/merge_requests/11017
From: Dmitry Timoshkov <dmitry@baikal.ru> Wine-Bug: http://bugs.winehq.org/show_bug.cgi?id=50583 Signed-off-by: Dmitry Timoshkov <dmitry@baikal.ru> --- dlls/msi/custom.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dlls/msi/custom.c b/dlls/msi/custom.c index cee8e06981c..ab25d1e848c 100644 --- a/dlls/msi/custom.c +++ b/dlls/msi/custom.c @@ -553,9 +553,9 @@ UINT CDECL __wine_msi_call_dll_function(DWORD client_pid, const GUID *guid) { r = custom_proc_wrapper( fn, hPackage ); } - __EXCEPT_PAGE_FAULT + __EXCEPT_ALL { - ERR( "Custom action (%s:%s) caused a page fault: %#lx\n", + ERR( "Custom action (%s:%s) caused an exception: %#lx\n", debugstr_w(dll), debugstr_a(proc), GetExceptionCode() ); r = ERROR_SUCCESS; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/11017
From: Dmitry Timoshkov <dmitry@baikal.ru> Signed-off-by: Dmitry Timoshkov <dmitry@baikal.ru> --- dlls/msi/custom.c | 2 +- dlls/msi/tests/action.c | 88 ++++++++++++++++++++++++++++++++++++++ dlls/msi/tests/custom.c | 16 +++++++ dlls/msi/tests/custom.spec | 2 + 4 files changed, 107 insertions(+), 1 deletion(-) diff --git a/dlls/msi/custom.c b/dlls/msi/custom.c index ab25d1e848c..7b0e9fed87e 100644 --- a/dlls/msi/custom.c +++ b/dlls/msi/custom.c @@ -557,7 +557,7 @@ UINT CDECL __wine_msi_call_dll_function(DWORD client_pid, const GUID *guid) { ERR( "Custom action (%s:%s) caused an exception: %#lx\n", debugstr_w(dll), debugstr_a(proc), GetExceptionCode() ); - r = ERROR_SUCCESS; + r = ERROR_INSTALL_FAILURE; } __ENDTRY; } diff --git a/dlls/msi/tests/action.c b/dlls/msi/tests/action.c index 82d950ce466..dc2d9ac83bb 100644 --- a/dlls/msi/tests/action.c +++ b/dlls/msi/tests/action.c @@ -1970,6 +1970,31 @@ static const char rep_install_exec_seq_dat[] = "UnpublishFeatures\t\t5300\n" "InstallFinalize\t\t6000\n"; +static const char exception_install_exec_seq_dat[] = + "Action\tCondition\tSequence\n" + "s72\tS255\tI2\n" + "InstallExecuteSequence\tAction\n" + "CostInitialize\t\t800\n" + "FileCost\t\t900\n" + "CostFinalize\t\t1000\n" + "InstallValidate\t\t1400\n" + "InstallInitialize\t\t1500\n" + "ProcessComponents\t\t1600\n" + "page_fault\tPAGEFAULT AND NOT REMOVE\t601\n" + "raise_exception\tEXCEPTION AND NOT REMOVE\t602\n" + "RegisterProduct\t\t700\n" + "PublishFeatures\t\t800\n" + "PublishProduct\t\t900\n" + "UnpublishFeatures\t\t1000\n" + "InstallFinalize\t\t1100\n"; + +static const char exception_custom_action_dat[] = + "Action\tType\tSource\tTarget\n" + "s72\ti2\tS64\tS0\n" + "CustomAction\tAction\n" + "page_fault\t65\tcustom.dll\tpage_fault\n" + "raise_exception\t65\tcustom.dll\traise_exception\n"; + static const msi_table env_tables[] = { ADD_TABLE(component), @@ -2371,6 +2396,19 @@ static const msi_table rep_tables[] = ADD_TABLE(media) }; +static const msi_table exception_tables[] = +{ + ADD_TABLE(component), + ADD_TABLE(directory), + ADD_TABLE(feature), + ADD_TABLE(feature_comp), + ADD_TABLE(file), + ADD_TABLE(exception_install_exec_seq), + ADD_TABLE(exception_custom_action), + ADD_TABLE(media), + ADD_TABLE(property) +}; + /* cabinet definitions */ /* make the max size large so there is only one cab file */ @@ -6541,6 +6579,54 @@ error: DeleteFileA(msifile); } +static void test_page_fault(void) +{ + UINT r; + + if (!is_process_elevated()) + { + skip("Process is limited\n"); + return; + } + + create_database(msifile, exception_tables, ARRAY_SIZE(exception_tables)); + + MsiSetInternalUI(INSTALLUILEVEL_NONE, NULL); + + r = MsiInstallProductA(msifile, "PAGEFAULT=1"); + if (r == ERROR_INSTALL_PACKAGE_REJECTED) + skip("Not enough rights to perform tests\n"); + else + todo_wine + ok(r == ERROR_INSTALL_FAILURE, "Expected ERROR_INSTALL_FAILURE, got %u\n", r); + + DeleteFileA(msifile); +} + +static void test_raise_exception(void) +{ + UINT r; + + if (!is_process_elevated()) + { + skip("Process is limited\n"); + return; + } + + create_database(msifile, exception_tables, ARRAY_SIZE(exception_tables)); + + MsiSetInternalUI(INSTALLUILEVEL_NONE, NULL); + + r = MsiInstallProductA(msifile, "EXCEPTION=1"); + if (r == ERROR_INSTALL_PACKAGE_REJECTED) + skip("Not enough rights to perform tests\n"); + else + todo_wine + ok(r == ERROR_INSTALL_FAILURE, "Expected ERROR_INSTALL_FAILURE, got %u\n", r); + + DeleteFileA(msifile); +} + static HANDLE get_admin_token(void) { TOKEN_ELEVATION_TYPE type; @@ -6647,6 +6733,8 @@ START_TEST(action) test_register_mime_info(); test_publish_assemblies(); test_remove_existing_products(); + test_page_fault(); + test_raise_exception(); DeleteFileA(log_file); SetCurrentDirectoryA(prev_path); diff --git a/dlls/msi/tests/custom.c b/dlls/msi/tests/custom.c index 8c5e7e4bbd0..5e4687fa789 100644 --- a/dlls/msi/tests/custom.c +++ b/dlls/msi/tests/custom.c @@ -1411,6 +1411,22 @@ UINT WINAPI async2(MSIHANDLE hinst) return ERROR_SUCCESS; } +UINT WINAPI page_fault(MSIHANDLE hinst) +{ + UINT *p = NULL; + + *(volatile UINT *)p = 0xdeadbeef; + + return ERROR_SUCCESS; +} + +UINT WINAPI raise_exception(MSIHANDLE hinst) +{ + RaiseException(0xdeadbeef, 0, 0, NULL); + + 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 5d78377a799..4f3f1cb298a 100644 --- a/dlls/msi/tests/custom.spec +++ b/dlls/msi/tests/custom.spec @@ -60,3 +60,5 @@ @ stdcall tl_absent(long) @ stdcall wrv_present(long) @ stdcall wrv_absent(long) +@ stdcall page_fault(long) +@ stdcall raise_exception(long) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/11017
From: Dmitry Timoshkov <dmitry@baikal.ru> Signed-off-by: Dmitry Timoshkov <dmitry@baikal.ru> --- dlls/msi/custom.c | 6 ++---- dlls/msi/tests/action.c | 2 -- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/dlls/msi/custom.c b/dlls/msi/custom.c index 7b0e9fed87e..d83cbe72be0 100644 --- a/dlls/msi/custom.c +++ b/dlls/msi/custom.c @@ -354,8 +354,7 @@ static UINT wait_process_handle(MSIPACKAGE* package, UINT type, msi_dialog_check_messages(ProcessHandle); - if (!(type & msidbCustomActionTypeContinue)) - rc = custom_get_process_return(ProcessHandle); + rc = custom_get_process_return(ProcessHandle); CloseHandle(ProcessHandle); } @@ -411,8 +410,7 @@ static UINT wait_thread_handle( custom_action_info *info ) msi_dialog_check_messages( info->handle ); - if (!(info->type & msidbCustomActionTypeContinue)) - rc = custom_get_thread_return( info->package, info->handle ); + rc = custom_get_thread_return( info->package, info->handle ); free_custom_action_data( info ); } diff --git a/dlls/msi/tests/action.c b/dlls/msi/tests/action.c index dc2d9ac83bb..93e5868b319 100644 --- a/dlls/msi/tests/action.c +++ b/dlls/msi/tests/action.c @@ -6597,7 +6597,6 @@ static void test_page_fault(void) if (r == ERROR_INSTALL_PACKAGE_REJECTED) skip("Not enough rights to perform tests\n"); else - todo_wine ok(r == ERROR_INSTALL_FAILURE, "Expected ERROR_INSTALL_FAILURE, got %u\n", r); DeleteFileA(msifile); @@ -6621,7 +6620,6 @@ static void test_raise_exception(void) if (r == ERROR_INSTALL_PACKAGE_REJECTED) skip("Not enough rights to perform tests\n"); else - todo_wine ok(r == ERROR_INSTALL_FAILURE, "Expected ERROR_INSTALL_FAILURE, got %u\n", r); DeleteFileA(msifile); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/11017
On Fri May 29 14:34:17 2026 +0000, Hans Leidekker wrote:
Looks good, thanks. Not sure about msidbCustomActionTypeContinue but here it's sufficient to know that the action fails. Please combine the test tables. You can use a condition variable in InstallExecuteSequence to select the custom action: ``` "page_fault\tPAGEFAULT AND NOT REMOVE\t601\n" "raise_exception\tEXCEPTION AND NOT REMOVE\t602\n" ``` And then install like this: ``` r = MsiInstallProductA(msifile, "PAGEFAULT=1"); ``` Thanks for the suggestions. I had to move the 'Catch all exceptions' patch to the start, otherwise the patch with the tests causes an unhandled exception.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/11017#note_141699
Hans Leidekker (@hans) commented about dlls/msi/custom.c:
msi_dialog_check_messages( info->handle );
- if (!(info->type & msidbCustomActionTypeContinue)) - rc = custom_get_thread_return( info->package, info->handle ); + rc = custom_get_thread_return( info->package, info->handle );
This removes msidbCustomActionTypeContinue handling and will break installers. The reason the test fails is not the exception but something else in the test installer: it also fails without the custom actions. I'll attach a fixed version with minimal InstallExecuteSequence and CustomAction table. I also added tests for custom actions without the msidbCustomActionTypeContinue flag and merged the test functions to avoid duplicating boilerplate code. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11017#note_141775
On Sat May 30 13:56:53 2026 +0000, Hans Leidekker wrote:
This removes msidbCustomActionTypeContinue handling and will break installers. The reason the test fails is not the exception but something else in the test installer: it also fails without the custom actions. I'll attach a fixed version with minimal InstallExecuteSequence and CustomAction table. I also added tests for custom actions without the msidbCustomActionTypeContinue flag and merged the test functions to avoid duplicating boilerplate code. [msi_tests.diff](/uploads/169e9590fbc6c7d05144ac959135526e/msi_tests.diff)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/11017#note_141776
On Sat May 30 13:58:11 2026 +0000, Hans Leidekker wrote:
[msi_tests.diff](/uploads/169e9590fbc6c7d05144ac959135526e/msi_tests.diff) Thank you very much for figuring it out!
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/11017#note_141784
participants (3)
-
Dmitry Timoshkov -
Dmitry Timoshkov (＠dmitry)
-
Hans Leidekker (＠hans)