[PATCH 0/4] MR5216: msi: Fix costing calculations a bit, and avoid overflow.
This work is based on Wine-Staging patch "msi-msi_vcl_get_cost" by Mark Jansen. That patch attempts to avoid overflow, although not particularly well, since the damage would already have been done by calculate_file_cost(). This patch series addresses the issue in both places. The Wine-Staging patch seems to have originally been written in an attempt to address a bug visible with (at least) some Microsoft Office and LibreOffice installers where file costs would render incorrectly. That bug is recorded as ReactOS bug 12229 [1], which report includes both the patch to vcl_get_cost(), and the patch fixing the actual bug, which did eventually made it upstream as b53957df2a73ae51ca6ebc1e88bb9c4ed8b8d274. [1] https://jira.reactos.org/browse/CORE-12229 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5216
From: Zebediah Figura <zfigura(a)codeweavers.com> --- dlls/msi/tests/package.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/dlls/msi/tests/package.c b/dlls/msi/tests/package.c index 75bb330db9d..9f15bc37520 100644 --- a/dlls/msi/tests/package.c +++ b/dlls/msi/tests/package.c @@ -8472,10 +8472,14 @@ static void test_costs(void) add_media_entry( hdb, "'1', '2', 'cabinet', '', '', ''"); create_file_table( hdb ); - add_file_entry( hdb, "'one.txt', 'one', 'one.txt', 4096, '', '', 8192, 1" ); + add_file_entry( hdb, "'a.txt', 'one', 'a.txt', 2048000000, '', '', 8192, 1" ); + add_file_entry( hdb, "'b.txt', 'one', 'b.txt', 2048000000, '', '', 8192, 1" ); + add_file_entry( hdb, "'c.txt', 'one', 'c.txt', 2048000000, '', '', 8192, 1" ); + add_file_entry( hdb, "'d.txt', 'one', 'd.txt', 4097, '', '', 8192, 1" ); + add_file_entry( hdb, "'e.txt', 'one', 'e.txt', 1, '', '', 8192, 1" ); create_component_table( hdb ); - add_component_entry( hdb, "'one', '{B2F86B9D-8447-4BC5-8883-750C45AA31CA}', 'TARGETDIR', 0, '', 'one.txt'" ); + add_component_entry( hdb, "'one', '{B2F86B9D-8447-4BC5-8883-750C45AA31CA}', 'TARGETDIR', 0, '', 'a.txt'" ); add_component_entry( hdb, "'two', '{62A09F6E-0B74-4829-BDB7-CAB66F42CCE8}', 'TARGETDIR', 0, '', ''" ); create_feature_table( hdb ); @@ -8603,7 +8607,7 @@ static void test_costs(void) ok( r == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got %u\n", r ); ok( len == 2, "expected len == 2, got %lu\n", len ); ok( drive[0], "expected a drive\n" ); - ok( cost && cost != 0xdead, "expected cost > 0, got %d\n", cost ); + todo_wine ok( cost == 12000024, "got %d\n", cost ); ok( !temp, "expected temp == 0, got %d\n", temp ); len = sizeof(drive); @@ -8647,7 +8651,7 @@ static void test_costs(void) cost = 0xdead; r = MsiGetFeatureCostA( hpkg, "one", MSICOSTTREE_SELFONLY, INSTALLSTATE_LOCAL, &cost ); ok( !r, "got %u\n", r); - ok( cost == 8, "got %d\n", cost ); + todo_wine ok( cost == 12000024, "got %d\n", cost ); MsiCloseHandle( hpkg ); error: -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5216
From: Zebediah Figura <zfigura(a)codeweavers.com> --- dlls/msi/msi.c | 4 ++-- dlls/msi/msipriv.h | 7 +++++++ dlls/msi/tests/package.c | 2 +- 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/dlls/msi/msi.c b/dlls/msi/msi.c index 2b821d08cf5..611399a8fb0 100644 --- a/dlls/msi/msi.c +++ b/dlls/msi/msi.c @@ -2037,14 +2037,14 @@ UINT WINAPI MsiEnumComponentCostsW( MSIHANDLE handle, const WCHAR *component, DW } else if ((file = msi_get_loaded_file( package, comp->KeyPath ))) { - *cost = max( 8, comp->Cost / 512 ); + *cost = cost_from_size( comp->Cost ); *buflen = set_drive( drive, file->TargetPath[0] ); r = ERROR_SUCCESS; } } else if (IStorage_Stat( package->db->storage, &stat, STATFLAG_NONAME ) == S_OK) { - *temp = max( 8, stat.cbSize.QuadPart / 512 ); + *temp = cost_from_size( stat.cbSize.QuadPart ); *buflen = set_drive( drive, path[0] ); r = ERROR_SUCCESS; } diff --git a/dlls/msi/msipriv.h b/dlls/msi/msipriv.h index 4c2f2ae2589..588aa9a7487 100644 --- a/dlls/msi/msipriv.h +++ b/dlls/msi/msipriv.h @@ -1194,4 +1194,11 @@ static inline LPWSTR strdupUtoW( LPCSTR str ) return ret; } +static inline int cost_from_size( int size ) +{ + /* Cost is size rounded up to the nearest 4096 bytes, + * expressed in units of 512 bytes. */ + return ((size + 4095) & ~4095) / 512; +} + #endif /* __WINE_MSI_PRIVATE__ */ diff --git a/dlls/msi/tests/package.c b/dlls/msi/tests/package.c index 9f15bc37520..9257a835891 100644 --- a/dlls/msi/tests/package.c +++ b/dlls/msi/tests/package.c @@ -8628,7 +8628,7 @@ static void test_costs(void) ok( len == 2, "expected len == 2, got %lu\n", len ); ok( drive[0], "expected a drive\n" ); ok( !cost, "expected cost == 0, got %d\n", cost ); - ok( temp && temp != 0xdead, "expected temp > 0, got %d\n", temp ); + todo_wine ok( temp && temp != 0xdead, "expected temp > 0, got %d\n", temp ); /* increased index */ len = sizeof(drive); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5216
From: Zebediah Figura <zfigura(a)codeweavers.com> This avoids overflow when component costs exceed 4 GB. --- dlls/msi/action.c | 14 +++++++------- dlls/msi/install.c | 3 +-- dlls/msi/msi.c | 4 ++-- dlls/msi/msipriv.h | 3 ++- dlls/msi/tests/package.c | 4 ++-- 5 files changed, 14 insertions(+), 14 deletions(-) diff --git a/dlls/msi/action.c b/dlls/msi/action.c index 20da818e686..06f399223f4 100644 --- a/dlls/msi/action.c +++ b/dlls/msi/action.c @@ -2070,7 +2070,7 @@ static UINT calculate_file_cost( MSIPACKAGE *package ) if (msi_get_file_attributes( package, file->TargetPath ) == INVALID_FILE_ATTRIBUTES) { - comp->Cost += file->FileSize; + comp->cost += cost_from_size( file->FileSize ); continue; } file_size = msi_get_disk_file_size( package, file->TargetPath ); @@ -2082,7 +2082,7 @@ static UINT calculate_file_cost( MSIPACKAGE *package ) { if (msi_compare_file_versions( file_version, file->Version ) < 0) { - comp->Cost += file->FileSize - file_size; + comp->cost += cost_from_size( file->FileSize - file_size ); } free( file_version ); continue; @@ -2091,7 +2091,7 @@ static UINT calculate_file_cost( MSIPACKAGE *package ) { if (msi_compare_font_versions( font_version, file->Version ) < 0) { - comp->Cost += file->FileSize - file_size; + comp->cost += cost_from_size( file->FileSize - file_size ); } free( font_version ); continue; @@ -2099,7 +2099,7 @@ static UINT calculate_file_cost( MSIPACKAGE *package ) } if (file_size != file->FileSize) { - comp->Cost += file->FileSize - file_size; + comp->cost += cost_from_size( file->FileSize - file_size ); } } @@ -2217,7 +2217,7 @@ static ULONGLONG get_volume_space_required( MSIPACKAGE *package ) LIST_FOR_EACH_ENTRY( comp, &package->components, MSICOMPONENT, entry ) { - if (comp->Action == INSTALLSTATE_LOCAL) ret += comp->Cost; + if (comp->Action == INSTALLSTATE_LOCAL) ret += comp->cost; } return ret; } @@ -2292,10 +2292,10 @@ static UINT ACTION_CostFinalize(MSIPACKAGE *package) msi_set_property( package->db, L"PrimaryVolumeSpaceAvailable", buf, -1 ); } required = get_volume_space_required( package ); - swprintf( buf, ARRAY_SIZE(buf), L"%lu", required / 512 ); + swprintf( buf, ARRAY_SIZE(buf), L"%lu", required ); msi_set_property( package->db, L"PrimaryVolumeSpaceRequired", buf, -1 ); - swprintf( buf, ARRAY_SIZE(buf), L"%lu", (free.QuadPart - required) / 512 ); + swprintf( buf, ARRAY_SIZE(buf), L"%lu", (free.QuadPart / 512) - required ); msi_set_property( package->db, L"PrimaryVolumeSpaceRemaining", buf, -1 ); msi_set_property( package->db, L"PrimaryVolumePath", primary_folder, 2 ); } diff --git a/dlls/msi/install.c b/dlls/msi/install.c index 609a3016eaa..f4db3b510f1 100644 --- a/dlls/msi/install.c +++ b/dlls/msi/install.c @@ -1147,7 +1147,7 @@ static INT feature_cost( MSIFEATURE *feature ) LIST_FOR_EACH_ENTRY( cl, &feature->Components, ComponentList, entry ) { - cost += cl->component->Cost; + cost += cl->component->cost; } return cost; } @@ -1197,7 +1197,6 @@ UINT MSI_GetFeatureCost( MSIPACKAGE *package, MSIFEATURE *feature, MSICOSTTREE t break; } - *cost /= 512; return ERROR_SUCCESS; } diff --git a/dlls/msi/msi.c b/dlls/msi/msi.c index 611399a8fb0..2c0f3aa50f5 100644 --- a/dlls/msi/msi.c +++ b/dlls/msi/msi.c @@ -2028,7 +2028,7 @@ UINT WINAPI MsiEnumComponentCostsW( MSIHANDLE handle, const WCHAR *component, DW GetWindowsDirectoryW( path, MAX_PATH ); if (component && component[0]) { - if (msi_is_global_assembly( comp )) *temp = comp->Cost; + if (msi_is_global_assembly( comp )) *temp = comp->cost; if (!comp->Enabled || !comp->KeyPath) { *cost = 0; @@ -2037,7 +2037,7 @@ UINT WINAPI MsiEnumComponentCostsW( MSIHANDLE handle, const WCHAR *component, DW } else if ((file = msi_get_loaded_file( package, comp->KeyPath ))) { - *cost = cost_from_size( comp->Cost ); + *cost = comp->cost; *buflen = set_drive( drive, file->TargetPath[0] ); r = ERROR_SUCCESS; } diff --git a/dlls/msi/msipriv.h b/dlls/msi/msipriv.h index 588aa9a7487..b7bc1e7b067 100644 --- a/dlls/msi/msipriv.h +++ b/dlls/msi/msipriv.h @@ -532,7 +532,8 @@ typedef struct tagMSICOMPONENT INSTALLSTATE Action; BOOL ForceLocalState; BOOL Enabled; - INT Cost; + /* Cost is in 512-byte units, as returned from MsiEnumComponentCosts() et al. */ + int cost; INT RefCount; LPWSTR FullKeypath; LPWSTR AdvertiseString; diff --git a/dlls/msi/tests/package.c b/dlls/msi/tests/package.c index 9257a835891..228fd91ef43 100644 --- a/dlls/msi/tests/package.c +++ b/dlls/msi/tests/package.c @@ -8607,7 +8607,7 @@ static void test_costs(void) ok( r == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got %u\n", r ); ok( len == 2, "expected len == 2, got %lu\n", len ); ok( drive[0], "expected a drive\n" ); - todo_wine ok( cost == 12000024, "got %d\n", cost ); + ok( cost == 12000024, "got %d\n", cost ); ok( !temp, "expected temp == 0, got %d\n", temp ); len = sizeof(drive); @@ -8651,7 +8651,7 @@ static void test_costs(void) cost = 0xdead; r = MsiGetFeatureCostA( hpkg, "one", MSICOSTTREE_SELFONLY, INSTALLSTATE_LOCAL, &cost ); ok( !r, "got %u\n", r); - todo_wine ok( cost == 12000024, "got %d\n", cost ); + ok( cost == 12000024, "got %d\n", cost ); MsiCloseHandle( hpkg ); error: -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5216
From: Zebediah Figura <zfigura(a)codeweavers.com> The main motivation here is to avoid overflow, and multiplying in one place seems simpler. --- dlls/msi/dialog.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/dlls/msi/dialog.c b/dlls/msi/dialog.c index f70b127a7a5..57c9cc9bc9e 100644 --- a/dlls/msi/dialog.c +++ b/dlls/msi/dialog.c @@ -3107,14 +3107,12 @@ static LONGLONG vcl_get_cost( msi_dialog *dialog ) if (ERROR_SUCCESS == (MSI_GetFeatureCost(dialog->package, feature, MSICOSTTREE_SELFONLY, INSTALLSTATE_LOCAL, &each_cost))) { - /* each_cost is in 512-byte units */ - total_cost += each_cost * 512; + total_cost += each_cost; } if (ERROR_SUCCESS == (MSI_GetFeatureCost(dialog->package, feature, MSICOSTTREE_SELFONLY, INSTALLSTATE_ABSENT, &each_cost))) { - /* each_cost is in 512-byte units */ - total_cost -= each_cost * 512; + total_cost -= each_cost; } } return total_cost; @@ -3131,7 +3129,7 @@ static void dialog_vcl_add_drives( msi_dialog *dialog, struct control *control ) DWORD size, flags; int i = 0; - cost = vcl_get_cost(dialog); + cost = vcl_get_cost(dialog) * 512; StrFormatByteSizeW(cost, cost_text, MAX_PATH); size = GetLogicalDriveStringsW( 0, NULL ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5216
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=143653 Your paranoid android. === debian11b (64 bit WoW report) === wldap32: parse.c:417: Test failed: ldap_connect failed 0x51 parse.c:427: Test failed: ldap_set_optionW should fail: 0 parse.c:431: Test failed: ldap_search_sA failed 0x51 parse.c:432: Test failed: expected res != NULL parse.c:602: Test failed: ldap_connect should succeed, got 0x51 parse.c:604: Test failed: ldap_start_tls_sA should fail, got 0x51 parse.c:205: Test failed: ldap_addA should succeed, got 0xffffffff parse.c:207: Test failed: ldap_addA should succeed, got 0xffffffff parse.c:209: Test failed: ldap_addA should succeed, got 0xffffffff parse.c:216: Test failed: ldap_add_sA should fail, got 0x51 parse.c:218: Test failed: ldap_add_sA should fail, got 0x51 parse.c:220: Test failed: ldap_add_sA should fail, got 0x51 parse.c:227: Test failed: ldap_add_extA should succeed, got 0x51 parse.c:229: Test failed: ldap_add_extA should succeed, got 0x51 parse.c:233: Test failed: ldap_add_extA should succeed, got 0x51 parse.c:240: Test failed: ldap_add_ext_sA should fail, got 0x51 parse.c:242: Test failed: ldap_add_ext_sA should fail, got 0x51 parse.c:244: Test failed: ldap_add_ext_sA should fail, got 0x51 parse.c:259: Test failed: ldap_modifyA should succeed, got 0xffffffff parse.c:261: Test failed: ldap_modifyA should succeed, got 0xffffffff parse.c:263: Test failed: ldap_modifyA should succeed, got 0xffffffff parse.c:270: Test failed: ldap_modify_sA should fail, got 0x51 parse.c:272: Test failed: ldap_modify_sA should fail, got 0x51 parse.c:274: Test failed: ldap_modify_sA should fail, got 0x51 parse.c:281: Test failed: ldap_modify_extA should succeed, got 0x51 parse.c:283: Test failed: ldap_modify_extA should succeed, got 0x51 parse.c:287: Test failed: ldap_modify_extA should succeed, got 0x51 parse.c:294: Test failed: ldap_modify_ext_sA should fail, got 0x51 parse.c:296: Test failed: ldap_modify_ext_sA should fail, got 0x51 parse.c:298: Test failed: ldap_modify_ext_sA should fail, got 0x51 parse.c:311: Test failed: ldap_compareA should succeed, got 0xffffffff parse.c:315: Test failed: ldap_compareA should succeed, got 0xffffffff parse.c:317: Test failed: ldap_compareA should succeed, got 0xffffffff parse.c:324: Test failed: ldap_compare_sA should fail, got 0x51 parse.c:328: Test failed: ldap_compare_sA should fail, got 0x51 parse.c:330: Test failed: ldap_compare_sA should fail, got 0x51 parse.c:337: Test failed: ldap_compare_extA should succeed, got 0x51 parse.c:341: Test failed: ldap_compare_extA should succeed, got 0x51 parse.c:343: Test failed: ldap_compare_extA should succeed, got 0x51 parse.c:345: Test failed: ldap_compare_extA should succeed, got 0x51 parse.c:349: Test failed: ldap_compare_extA should succeed, got 0x51 parse.c:356: Test failed: ldap_compare_ext_sA should fail, got 0x51 parse.c:360: Test failed: ldap_compare_ext_sA should fail, got 0x51 parse.c:362: Test failed: ldap_compare_ext_sA should fail, got 0x51 parse.c:364: Test failed: ldap_compare_ext_sA should fail, got 0x51 parse.c:54: Test failed: ldap_create_sort_controlA failed 0x51 Unhandled exception: page fault on read access to 0xffffffffffffffff in 64-bit code (0x006fffffc98fa0).
This merge request was approved by Hans Leidekker. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5216
Is there's any code from Mark left in these patches? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5216#note_63337
Is there's any code from Mark left in these patches?
Not really. 4/4 is the patch that actually corresponds to his but I changed it in a different place. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5216#note_63415
For convenience, here is Mark's patch: https://gitlab.winehq.org/wine/wine-staging/-/blob/master/patches/msi-msi_vc... -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5216#note_63416
participants (4)
-
Hans Leidekker (＠hans) -
Marvin -
Zebediah Figura
-
Zebediah Figura (＠zfigura)