[PATCH v2 0/2] MR6760: msi: Make MsiInstallProductW open msi readonly
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=57343 This is what the Java JRE installer does: - It calls MsiInstallProductW - this opens the msi read/write - The msi runs installer.exe - installer.exe tries to open the msi readonly -- v2: msi: Make MsiInstallProductW open msi readonly msi/tests: Add test for MsiOpenDatabaseA during MsiInstallProductA https://gitlab.winehq.org/wine/wine/-/merge_requests/6760
From: Fabian Maurer <dark.shadow4(a)web.de> --- dlls/msi/tests/custom.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/dlls/msi/tests/custom.c b/dlls/msi/tests/custom.c index 8c5e7e4bbd0..3a3154da7da 100644 --- a/dlls/msi/tests/custom.c +++ b/dlls/msi/tests/custom.c @@ -1259,6 +1259,23 @@ static void test_view_get_error(MSIHANDLE hinst) MsiCloseHandle(db); } +static void test_open_msi(MSIHANDLE hinst) +{ + MSIHANDLE handle = 0; + UINT result; + char buffer[300]; + DWORD len = sizeof(buffer); + + result = MsiGetPropertyA(hinst, "DATABASE", buffer, &len); + ok(hinst, result == 0, "Failed: %x\n", result); + + result = MsiOpenDatabaseA(buffer, (LPCSTR)MSIDBOPEN_READONLY, &handle); + todo_wine + ok (hinst, result == 0, "Got %u\n", result); + + MsiCloseHandle(handle); +} + /* Main test. Anything that doesn't depend on a specific install configuration * or have undesired side effects should go here. */ UINT WINAPI main_test(MSIHANDLE hinst) @@ -1287,6 +1304,7 @@ UINT WINAPI main_test(MSIHANDLE hinst) test_costs(hinst); test_invalid_functions(hinst); test_view_get_error(hinst); + test_open_msi(hinst); return ERROR_SUCCESS; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6760
From: Fabian Maurer <dark.shadow4(a)web.de> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=57343 --- dlls/msi/msi.c | 2 +- dlls/msi/msipriv.h | 1 + dlls/msi/package.c | 5 +++-- dlls/msi/tests/custom.c | 1 - 4 files changed, 5 insertions(+), 4 deletions(-) diff --git a/dlls/msi/msi.c b/dlls/msi/msi.c index 2c0f3aa50f5..966bf8f98c6 100644 --- a/dlls/msi/msi.c +++ b/dlls/msi/msi.c @@ -229,7 +229,7 @@ UINT WINAPI MsiInstallProductW(LPCWSTR szPackagePath, LPCWSTR szCommandLine) { MSIPACKAGE *package = NULL; const WCHAR *reinstallmode; - DWORD options = 0; + DWORD options = WINE_OPENPACKAGEFLAGS_READONLY; UINT r, len; TRACE("%s %s\n",debugstr_w(szPackagePath), debugstr_w(szCommandLine)); diff --git a/dlls/msi/msipriv.h b/dlls/msi/msipriv.h index e20ead557ce..b9eb4c502bf 100644 --- a/dlls/msi/msipriv.h +++ b/dlls/msi/msipriv.h @@ -906,6 +906,7 @@ extern UINT MSI_SetInstallLevel( MSIPACKAGE *package, int iInstallLevel ); /* package internals */ #define WINE_OPENPACKAGEFLAGS_RECACHE 0x80000000 +#define WINE_OPENPACKAGEFLAGS_READONLY 0x40000000 extern MSIPACKAGE *MSI_CreatePackage( MSIDATABASE * ); extern UINT MSI_OpenPackageW( LPCWSTR szPackage, DWORD dwOptions, MSIPACKAGE **pPackage ); extern UINT MSI_SetTargetPathW( MSIPACKAGE *, LPCWSTR, LPCWSTR ); diff --git a/dlls/msi/package.c b/dlls/msi/package.c index 89b4c09dfa4..02775a16d4f 100644 --- a/dlls/msi/package.c +++ b/dlls/msi/package.c @@ -1327,7 +1327,7 @@ UINT MSI_OpenPackageW(LPCWSTR szPackage, DWORD dwOptions, MSIPACKAGE **pPackage) MSIRECORD *data_row, *info_row; UINT r; WCHAR localfile[MAX_PATH], cachefile[MAX_PATH]; - LPCWSTR file = szPackage; + LPCWSTR file = szPackage, mode; DWORD index = 0; MSISUMMARYINFO *si; BOOL delete_on_close = FALSE; @@ -1405,7 +1405,8 @@ UINT MSI_OpenPackageW(LPCWSTR szPackage, DWORD dwOptions, MSIPACKAGE **pPackage) product_version = get_product_version( db ); msiobj_release( &db->hdr ); TRACE("opening package %s\n", debugstr_w( localfile )); - r = MSI_OpenDatabaseW( localfile, MSIDBOPEN_TRANSACT, &db ); + mode = (dwOptions & WINE_OPENPACKAGEFLAGS_READONLY) ? MSIDBOPEN_READONLY : MSIDBOPEN_TRANSACT; + r = MSI_OpenDatabaseW( localfile, mode, &db ); if (r != ERROR_SUCCESS) { free( product_version ); diff --git a/dlls/msi/tests/custom.c b/dlls/msi/tests/custom.c index 3a3154da7da..f0980274ce3 100644 --- a/dlls/msi/tests/custom.c +++ b/dlls/msi/tests/custom.c @@ -1270,7 +1270,6 @@ static void test_open_msi(MSIHANDLE hinst) ok(hinst, result == 0, "Failed: %x\n", result); result = MsiOpenDatabaseA(buffer, (LPCSTR)MSIDBOPEN_READONLY, &handle); - todo_wine ok (hinst, result == 0, "Got %u\n", result); MsiCloseHandle(handle); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6760
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=149753 Your paranoid android. === debian11b (64 bit WoW report) === user32: input.c:4305: Test succeeded inside todo block: button_down_hwnd_todo 1: got MSG_TEST_WIN hwnd 00000000013C00F4, msg WM_LBUTTONDOWN, wparam 0x1, lparam 0x320032 win.c:4070: Test failed: Expected active window 000000000374018C, got 0000000000000000. win.c:4071: Test failed: Expected focus window 000000000374018C, got 0000000000000000.
On Tue Nov 19 01:46:45 2024 +0000, Jacek Caban wrote:
I think that the ieframe should be a separate MR. Thanks, split off. An idea how I should continue for the msi part?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6760#note_87954
Any news on this? Not sure what I would need to change here -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6760#note_119940
Nikolay Sivov (@nsivov) commented about dlls/msi/tests/custom.c:
MsiCloseHandle(db); }
+static void test_open_msi(MSIHANDLE hinst) +{ + MSIHANDLE handle = 0; + UINT result; + char buffer[300]; + DWORD len = sizeof(buffer); + + result = MsiGetPropertyA(hinst, "DATABASE", buffer, &len); + ok(hinst, result == 0, "Failed: %x\n", result);
Why do you need a comma operator here? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6760#note_119946
Regarding what you need to change, there was a comment about tests https://gitlab.winehq.org/wine/wine/-/merge_requests/6760#note_86571, and I don't think this was addressed in any way. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6760#note_119947
On Wed Oct 29 08:15:30 2025 +0000, Nikolay Sivov wrote:
Why do you need a comma operator here? There shouldn't be a comma operator here.
msi uses a different `ok` function: ```#define ok(hinst, condition, ...) ok_(hinst, todo_level, __FILE__, __LINE__, condition, __VA_ARGS__)``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6760#note_120022
On Wed Oct 29 18:56:50 2025 +0000, Nikolay Sivov wrote:
Regarding what you need to change, there was a comment about tests https://gitlab.winehq.org/wine/wine/-/merge_requests/6760#note_86571, and I don't think this was addressed in any way. I asked for clarification, since I don't really understand what they mean. And rereading it now, unfortunately I still don't get it. The sharing violation happens because of CreateFileW, but CreateFileW behavior should be correct. I could add more tests or recheck the logic, but I'd need to understand what exactly.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6760#note_120023
On Wed Oct 29 18:56:50 2025 +0000, Fabian Maurer wrote:
I asked for clarification, since I don't really understand what they mean. And rereading it now, unfortunately I still don't get it. The sharing violation happens because of CreateFileW, but CreateFileW behavior should be correct. I could add more tests or recheck the logic, but I'd need to understand what exactly. Sure, CreateFile() behavior should be correct but we're mapping msi flags like MSI_OPEN_READONLY to OLE storage flags like STGM_DIRECT|STGM_READ|STGM_SHARE_DENY_WRITE, which in turn are mapped to CreateFile() flags. Something could be wrong there, so we'd want to verify these mappings. I also mentioned that patching msi files depends on the the local copy being writable. Native may not depend on it, or perhaps it doesn't keep the file open that way.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6760#note_120025
On Wed Oct 29 19:39:19 2025 +0000, Hans Leidekker wrote:
Sure, CreateFile() behavior should be correct but we're mapping msi flags like MSI_OPEN_READONLY to OLE storage flags like STGM_DIRECT|STGM_READ|STGM_SHARE_DENY_WRITE, which in turn are mapped to CreateFile() flags. Something could be wrong there, so we'd want to verify these mappings. I also mentioned that patching msi files depends on the the local copy being writable. Native may not depend on it, or perhaps it doesn't keep the file open that way. Hm I see, but how can I verify that mapping? Sorry I currently don't see how
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6760#note_120079
This fixed [57343](https://bugs.winehq.org/show_bug.cgi?id=57343 "NEW - Java jre-8u431-windows-x64 crashes during install with null-pointer dereference") (on Wine 9.17) and the duplicate [59777](https://bugs.winehq.org/show_bug.cgi?id=59777 "RESOLVED DUPLICATE - Unhandled exception: page fault on read access to 0x0000000000000020 in 64-bit code (0x000001400575a7)") (on Wine 11.9) for me. 👍 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6760#note_140855
participants (6)
-
Fabian Maurer -
Fabian Maurer (＠DarkShadow44)
-
Geremia (＠Geremia) -
Hans Leidekker (＠hans)
-
Marvin -
Nikolay Sivov (＠nsivov)