[PATCH 0/2] MR5229: ole32: Do not lock storage in read-only deny-write mode.
(STGM_READ | STGM_SHARE_DENY_WRITE) doesn't apply to 20 user limit https://docs.microsoft.com/en-us/windows/win32/stg/structured-storage-interf... -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5229
From: Daniel Lehman <dlehman25(a)gmail.com> --- dlls/ole32/storage32.c | 8 +++++--- dlls/ole32/tests/storage32.c | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/dlls/ole32/storage32.c b/dlls/ole32/storage32.c index 1bf08e45701..78e6de22be8 100644 --- a/dlls/ole32/storage32.c +++ b/dlls/ole32/storage32.c @@ -5036,7 +5036,7 @@ static HRESULT StorageImpl_GrabLocks(StorageImpl *This, DWORD openFlags) ULARGE_INTEGER offset; ULARGE_INTEGER cb; DWORD share_mode = STGM_SHARE_MODE(openFlags); - BOOL supported; + BOOL supported, ro_denyw; if (openFlags & STGM_NOSNAPSHOT) { @@ -5045,6 +5045,8 @@ static HRESULT StorageImpl_GrabLocks(StorageImpl *This, DWORD openFlags) else if (share_mode != STGM_SHARE_EXCLUSIVE) share_mode = STGM_SHARE_DENY_WRITE; } + ro_denyw = (STGM_ACCESS_MODE(openFlags) == STGM_READ) && (share_mode == STGM_SHARE_DENY_WRITE); + /* Wrap all other locking inside a single lock so we can check ranges safely */ offset.QuadPart = RANGELOCK_CHECKLOCKS; cb.QuadPart = 1; @@ -5088,7 +5090,7 @@ static HRESULT StorageImpl_GrabLocks(StorageImpl *This, DWORD openFlags) hr = StorageImpl_LockOne(This, RANGELOCK_PRIORITY2_FIRST, RANGELOCK_PRIORITY2_LAST); } - if (SUCCEEDED(hr) && (STGM_ACCESS_MODE(openFlags) != STGM_WRITE)) + if (SUCCEEDED(hr) && (STGM_ACCESS_MODE(openFlags) != STGM_WRITE) && !ro_denyw) hr = StorageImpl_LockOne(This, RANGELOCK_READ_FIRST, RANGELOCK_READ_LAST); if (SUCCEEDED(hr) && (STGM_ACCESS_MODE(openFlags) != STGM_READ)) @@ -5097,7 +5099,7 @@ static HRESULT StorageImpl_GrabLocks(StorageImpl *This, DWORD openFlags) if (SUCCEEDED(hr) && (share_mode == STGM_SHARE_DENY_READ || share_mode == STGM_SHARE_EXCLUSIVE)) hr = StorageImpl_LockOne(This, RANGELOCK_DENY_READ_FIRST, RANGELOCK_DENY_READ_LAST); - if (SUCCEEDED(hr) && (share_mode == STGM_SHARE_DENY_WRITE || share_mode == STGM_SHARE_EXCLUSIVE)) + if (SUCCEEDED(hr) && (share_mode == STGM_SHARE_DENY_WRITE || share_mode == STGM_SHARE_EXCLUSIVE) && !ro_denyw) hr = StorageImpl_LockOne(This, RANGELOCK_DENY_WRITE_FIRST, RANGELOCK_DENY_WRITE_LAST); if (SUCCEEDED(hr) && (openFlags & STGM_NOSNAPSHOT) == STGM_NOSNAPSHOT) diff --git a/dlls/ole32/tests/storage32.c b/dlls/ole32/tests/storage32.c index 97809db539c..9728c1c9db8 100644 --- a/dlls/ole32/tests/storage32.c +++ b/dlls/ole32/tests/storage32.c @@ -3394,7 +3394,7 @@ static const struct lock_test lock_tests[] = { { STGM_READWRITE|STGM_TRANSACTED, FALSE, GENERIC_READ|GENERIC_WRITE, FILE_SHARE_READ|FILE_SHARE_WRITE, rw_locked_bytes, rw_fail_ranges, FALSE }, { STGM_READWRITE|STGM_TRANSACTED|STGM_NOSNAPSHOT, FALSE, GENERIC_READ|GENERIC_WRITE, FILE_SHARE_READ|FILE_SHARE_WRITE, nosn_locked_bytes, rwdw_fail_ranges, FALSE }, { STGM_READWRITE|STGM_TRANSACTED|STGM_SHARE_DENY_WRITE, FALSE, GENERIC_READ|GENERIC_WRITE, FILE_SHARE_READ, rwdw_locked_bytes, rwdw_fail_ranges, FALSE }, - { STGM_READ|STGM_SHARE_DENY_WRITE, FALSE, GENERIC_READ, FILE_SHARE_READ, no_locked_bytes, dw_fail_ranges, TRUE }, + { STGM_READ|STGM_SHARE_DENY_WRITE, FALSE, GENERIC_READ, FILE_SHARE_READ, no_locked_bytes, dw_fail_ranges, FALSE }, { STGM_READ|STGM_TRANSACTED, FALSE, GENERIC_READ, FILE_SHARE_READ|FILE_SHARE_WRITE, tr_locked_bytes, tr_fail_ranges, FALSE }, { STGM_READ|STGM_SHARE_EXCLUSIVE, FALSE, GENERIC_READ, FILE_SHARE_READ, roex_locked_bytes, roex_fail_ranges, FALSE }, { STGM_READ|STGM_SHARE_EXCLUSIVE|STGM_TRANSACTED, FALSE, GENERIC_READ, FILE_SHARE_READ, roex_locked_bytes, roex_fail_ranges, FALSE }, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5229
From: Daniel Lehman <dlehman25(a)gmail.com> --- dlls/ole32/tests/storage32.c | 115 ++++++++++------------------------- 1 file changed, 33 insertions(+), 82 deletions(-) diff --git a/dlls/ole32/tests/storage32.c b/dlls/ole32/tests/storage32.c index 9728c1c9db8..0b75affdf8a 100644 --- a/dlls/ole32/tests/storage32.c +++ b/dlls/ole32/tests/storage32.c @@ -3361,7 +3361,6 @@ struct lock_test DWORD sharing; const int *locked_bytes; const int *fail_ranges; - BOOL todo; }; static const int priority_locked_bytes[] = { 0x158, 0x181, 0x193, -1 }; @@ -3383,21 +3382,21 @@ static const int pr_fail_ranges[] = { 0x180,0x181, 0x1bb,0x1cf, -1 }; static const int roex_fail_ranges[] = { 0x0,-1 }; static const struct lock_test lock_tests[] = { - { STGM_PRIORITY, FALSE, GENERIC_READ, FILE_SHARE_READ|FILE_SHARE_WRITE, priority_locked_bytes, pr_fail_ranges, FALSE }, - { STGM_CREATE|STGM_SHARE_EXCLUSIVE|STGM_READWRITE, TRUE, GENERIC_READ|GENERIC_WRITE, FILE_SHARE_READ, rwex_locked_bytes, 0, FALSE }, - { STGM_CREATE|STGM_SHARE_EXCLUSIVE|STGM_READWRITE|STGM_TRANSACTED, TRUE, GENERIC_READ|GENERIC_WRITE, FILE_SHARE_READ, rwex_locked_bytes, 0, FALSE }, - { STGM_CREATE|STGM_READWRITE|STGM_TRANSACTED, TRUE, GENERIC_READ|GENERIC_WRITE, FILE_SHARE_READ|FILE_SHARE_WRITE, rw_locked_bytes, 0, FALSE }, - { STGM_CREATE|STGM_READWRITE|STGM_SHARE_DENY_WRITE|STGM_TRANSACTED, TRUE, GENERIC_READ|GENERIC_WRITE, FILE_SHARE_READ, rwdw_locked_bytes, 0, FALSE }, - { STGM_CREATE|STGM_WRITE|STGM_SHARE_DENY_WRITE|STGM_TRANSACTED, TRUE, GENERIC_READ|GENERIC_WRITE, FILE_SHARE_READ, wodw_locked_bytes, 0, FALSE }, - { STGM_SHARE_EXCLUSIVE|STGM_READWRITE, FALSE, GENERIC_READ|GENERIC_WRITE, FILE_SHARE_READ, rwex_locked_bytes, rwex_fail_ranges, FALSE }, - { STGM_SHARE_EXCLUSIVE|STGM_READWRITE|STGM_TRANSACTED, FALSE, GENERIC_READ|GENERIC_WRITE, FILE_SHARE_READ, rwex_locked_bytes, rwex_fail_ranges, FALSE }, - { STGM_READWRITE|STGM_TRANSACTED, FALSE, GENERIC_READ|GENERIC_WRITE, FILE_SHARE_READ|FILE_SHARE_WRITE, rw_locked_bytes, rw_fail_ranges, FALSE }, - { STGM_READWRITE|STGM_TRANSACTED|STGM_NOSNAPSHOT, FALSE, GENERIC_READ|GENERIC_WRITE, FILE_SHARE_READ|FILE_SHARE_WRITE, nosn_locked_bytes, rwdw_fail_ranges, FALSE }, - { STGM_READWRITE|STGM_TRANSACTED|STGM_SHARE_DENY_WRITE, FALSE, GENERIC_READ|GENERIC_WRITE, FILE_SHARE_READ, rwdw_locked_bytes, rwdw_fail_ranges, FALSE }, - { STGM_READ|STGM_SHARE_DENY_WRITE, FALSE, GENERIC_READ, FILE_SHARE_READ, no_locked_bytes, dw_fail_ranges, FALSE }, - { STGM_READ|STGM_TRANSACTED, FALSE, GENERIC_READ, FILE_SHARE_READ|FILE_SHARE_WRITE, tr_locked_bytes, tr_fail_ranges, FALSE }, - { STGM_READ|STGM_SHARE_EXCLUSIVE, FALSE, GENERIC_READ, FILE_SHARE_READ, roex_locked_bytes, roex_fail_ranges, FALSE }, - { STGM_READ|STGM_SHARE_EXCLUSIVE|STGM_TRANSACTED, FALSE, GENERIC_READ, FILE_SHARE_READ, roex_locked_bytes, roex_fail_ranges, FALSE }, + { STGM_PRIORITY, FALSE, GENERIC_READ, FILE_SHARE_READ|FILE_SHARE_WRITE, priority_locked_bytes, pr_fail_ranges }, + { STGM_CREATE|STGM_SHARE_EXCLUSIVE|STGM_READWRITE, TRUE, GENERIC_READ|GENERIC_WRITE, FILE_SHARE_READ, rwex_locked_bytes, 0 }, + { STGM_CREATE|STGM_SHARE_EXCLUSIVE|STGM_READWRITE|STGM_TRANSACTED, TRUE, GENERIC_READ|GENERIC_WRITE, FILE_SHARE_READ, rwex_locked_bytes, 0 }, + { STGM_CREATE|STGM_READWRITE|STGM_TRANSACTED, TRUE, GENERIC_READ|GENERIC_WRITE, FILE_SHARE_READ|FILE_SHARE_WRITE, rw_locked_bytes, 0 }, + { STGM_CREATE|STGM_READWRITE|STGM_SHARE_DENY_WRITE|STGM_TRANSACTED, TRUE, GENERIC_READ|GENERIC_WRITE, FILE_SHARE_READ, rwdw_locked_bytes, 0 }, + { STGM_CREATE|STGM_WRITE|STGM_SHARE_DENY_WRITE|STGM_TRANSACTED, TRUE, GENERIC_READ|GENERIC_WRITE, FILE_SHARE_READ, wodw_locked_bytes, 0 }, + { STGM_SHARE_EXCLUSIVE|STGM_READWRITE, FALSE, GENERIC_READ|GENERIC_WRITE, FILE_SHARE_READ, rwex_locked_bytes, rwex_fail_ranges }, + { STGM_SHARE_EXCLUSIVE|STGM_READWRITE|STGM_TRANSACTED, FALSE, GENERIC_READ|GENERIC_WRITE, FILE_SHARE_READ, rwex_locked_bytes, rwex_fail_ranges }, + { STGM_READWRITE|STGM_TRANSACTED, FALSE, GENERIC_READ|GENERIC_WRITE, FILE_SHARE_READ|FILE_SHARE_WRITE, rw_locked_bytes, rw_fail_ranges }, + { STGM_READWRITE|STGM_TRANSACTED|STGM_NOSNAPSHOT, FALSE, GENERIC_READ|GENERIC_WRITE, FILE_SHARE_READ|FILE_SHARE_WRITE, nosn_locked_bytes, rwdw_fail_ranges }, + { STGM_READWRITE|STGM_TRANSACTED|STGM_SHARE_DENY_WRITE, FALSE, GENERIC_READ|GENERIC_WRITE, FILE_SHARE_READ, rwdw_locked_bytes, rwdw_fail_ranges }, + { STGM_READ|STGM_SHARE_DENY_WRITE, FALSE, GENERIC_READ, FILE_SHARE_READ, no_locked_bytes, dw_fail_ranges }, + { STGM_READ|STGM_TRANSACTED, FALSE, GENERIC_READ, FILE_SHARE_READ|FILE_SHARE_WRITE, tr_locked_bytes, tr_fail_ranges }, + { STGM_READ|STGM_SHARE_EXCLUSIVE, FALSE, GENERIC_READ, FILE_SHARE_READ, roex_locked_bytes, roex_fail_ranges }, + { STGM_READ|STGM_SHARE_EXCLUSIVE|STGM_TRANSACTED, FALSE, GENERIC_READ, FILE_SHARE_READ, roex_locked_bytes, roex_fail_ranges }, }; static BOOL can_open(LPCWSTR filename, DWORD access, DWORD sharing) @@ -3414,58 +3413,28 @@ static BOOL can_open(LPCWSTR filename, DWORD access, DWORD sharing) } static void check_sharing(LPCWSTR filename, const struct lock_test *current, - DWORD access, DWORD sharing, const char *desc, DWORD *open_mode, BOOL *any_failure) + DWORD access, DWORD sharing, const char *desc, DWORD *open_mode) { if (can_open(filename, access, FILE_SHARE_READ|FILE_SHARE_WRITE|FILE_SHARE_DELETE)) { *open_mode = access; - if (!current->todo || (current->sharing & sharing)) - ok(current->sharing & sharing || - broken(!(current->sharing & sharing) && access == GENERIC_WRITE && (current->stg_mode & 0xf) != STGM_READ) /* win2k */, - "file with mode %lx should not be openable with %s permission\n", current->stg_mode, desc); - else - { - todo_wine ok(current->sharing & sharing || - broken(!(current->sharing & sharing) && access == GENERIC_WRITE && (current->stg_mode & 0xf) != STGM_READ) /* win2k */, - "file with mode %lx should not be openable with %s permission\n", current->stg_mode, desc); - *any_failure = TRUE; - } + ok(current->sharing & sharing || + broken(!(current->sharing & sharing) && access == GENERIC_WRITE && (current->stg_mode & 0xf) != STGM_READ) /* win2k */, + "file with mode %lx should not be openable with %s permission\n", current->stg_mode, desc); } else { - if (!current->todo || !(current->sharing & sharing)) - ok(!(current->sharing & sharing), "file with mode %lx should be openable with %s permission\n", current->stg_mode, desc); - else - { - todo_wine ok(!(current->sharing & sharing), "file with mode %lx should be openable with %s permission\n", current->stg_mode, desc); - *any_failure = TRUE; - } + ok(!(current->sharing & sharing), "file with mode %lx should be openable with %s permission\n", current->stg_mode, desc); } } static void check_access(LPCWSTR filename, const struct lock_test *current, - DWORD access, DWORD sharing, const char *desc, DWORD open_mode, BOOL *any_failure) + DWORD access, DWORD sharing, const char *desc, DWORD open_mode) { if (can_open(filename, open_mode, (FILE_SHARE_READ|FILE_SHARE_WRITE|FILE_SHARE_DELETE) & ~sharing)) - { - if (!current->todo || !(current->access & access)) - ok(!(current->access & access), "file with mode %lx should not be openable without %s sharing\n", current->stg_mode, desc); - else - { - todo_wine ok(!(current->access & access), "file with mode %lx should not be openable without %s sharing\n", current->stg_mode, desc); - *any_failure = TRUE; - } - } + ok(!(current->access & access), "file with mode %lx should not be openable without %s sharing\n", current->stg_mode, desc); else - { - if (!current->todo || (current->access & access)) - ok(current->access & access, "file with mode %lx should be openable without %s sharing\n", current->stg_mode, desc); - else - { - todo_wine ok(current->access & access, "file with mode %lx should be openable without %s sharing\n", current->stg_mode, desc); - *any_failure = TRUE; - } - } + ok(current->access & access, "file with mode %lx should be openable without %s sharing\n", current->stg_mode, desc); } static void test_locking(void) @@ -3478,7 +3447,6 @@ static void test_locking(void) for (i = 0; i < ARRAY_SIZE(lock_tests); i++) { const struct lock_test *current = &lock_tests[i]; - BOOL any_failure = FALSE; DWORD open_mode = 0; if (current->create) @@ -3503,9 +3471,9 @@ static void test_locking(void) } } - check_sharing(filename, current, GENERIC_READ, FILE_SHARE_READ, "READ", &open_mode, &any_failure); - check_sharing(filename, current, GENERIC_WRITE, FILE_SHARE_WRITE, "WRITE", &open_mode, &any_failure); - check_sharing(filename, current, DELETE, FILE_SHARE_DELETE, "DELETE", &open_mode, &any_failure); + check_sharing(filename, current, GENERIC_READ, FILE_SHARE_READ, "READ", &open_mode); + check_sharing(filename, current, GENERIC_WRITE, FILE_SHARE_WRITE, "WRITE", &open_mode); + check_sharing(filename, current, DELETE, FILE_SHARE_DELETE, "DELETE", &open_mode); if (open_mode != 0) { @@ -3514,9 +3482,9 @@ static void test_locking(void) OVERLAPPED ol; const int* next_lock = current->locked_bytes; - check_access(filename, current, GENERIC_READ, FILE_SHARE_READ, "READ", open_mode, &any_failure); - check_access(filename, current, GENERIC_WRITE, FILE_SHARE_WRITE, "WRITE", open_mode, &any_failure); - check_access(filename, current, DELETE, FILE_SHARE_DELETE, "DELETE", open_mode, &any_failure); + check_access(filename, current, GENERIC_READ, FILE_SHARE_READ, "READ", open_mode); + check_access(filename, current, GENERIC_WRITE, FILE_SHARE_WRITE, "WRITE", open_mode); + check_access(filename, current, DELETE, FILE_SHARE_DELETE, "DELETE", open_mode); hfile = CreateFileW(filename, open_mode, FILE_SHARE_READ|FILE_SHARE_WRITE|FILE_SHARE_DELETE, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL); ok(hfile != INVALID_HANDLE_VALUE, "couldn't open file with mode %lx\n", current->stg_mode); @@ -3544,15 +3512,8 @@ static void test_locking(void) else expect_locked = FALSE; - if (!current->todo || locked == expect_locked) - ok(locked == expect_locked, "byte %lx of file with mode %lx is %slocked but should %sbe\n", - ol.Offset, current->stg_mode, locked?"":"not ", expect_locked?"":"not "); - else - { - any_failure = TRUE; - todo_wine ok(locked == expect_locked, "byte %lx of file with mode %lx is %slocked but should %sbe\n", - ol.Offset, current->stg_mode, locked?"":"not ", expect_locked?"":"not "); - } + ok(locked == expect_locked, "byte %lx of file with mode %lx is %slocked but should %sbe\n", + ol.Offset, current->stg_mode, locked?"":"not ", expect_locked?"":"not "); } CloseHandle(hfile); @@ -3603,24 +3564,14 @@ static void test_locking(void) next_range += 2; } - if (!current->todo || failed == expect_failed) - ok(failed == expect_failed, "open with byte %lx locked, mode %lx %s but should %s\n", - ol.Offset, current->stg_mode, failed?"failed":"succeeded", expect_failed?"fail":"succeed"); - else - { - any_failure = TRUE; - todo_wine ok(failed == expect_failed, "open with byte %lx locked, mode %lx %s but should %s\n", - ol.Offset, current->stg_mode, failed?"failed":"succeeded", expect_failed?"fail":"succeed"); - } + ok(failed == expect_failed, "open with byte %lx locked, mode %lx %s but should %s\n", + ol.Offset, current->stg_mode, failed?"failed":"succeeded", expect_failed?"fail":"succeed"); } CloseHandle(hfile); } DeleteFileW(filename); - - if (current->todo && !any_failure) - todo_wine ok(1, "tests succeeded for mode %lx\n", current->stg_mode); } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5229
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=143700 Your paranoid android. === debian11b (64 bit WoW report) === wmvcore: wmvcore.c:1705: Test failed: Stream 0: Format 1: Got hr 0xc00d0041. wmvcore.c:1722: Test failed: Stream 0: Format 1: Media types didn't match.
This merge request was approved by Esme Povirk. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5229
participants (4)
-
Daniel Lehman -
Daniel Lehman (＠dlehman25)
-
Esme Povirk (＠madewokherd)
-
Marvin