[PATCH v3 0/1] MR6347: advpack: DelNode support the ADN_DEL_IF_EMPTY flag
Fixes a bug where `rundll32.exe advpack.dll,DelNodeRunDLL32 "c:\test",1` is only supposed to delete "test" if the directory is empty. Also causes the [DelDirs Inf section](https://www.mdgx.com/INF_web/deldirs.htm) to work correctly. Notes: - I changed the code so it ignores the result of `SetFileAttributesW` because the user might not have FILE_WRITE_ATTRIBUTES even if they have DELETE rights. This seems to match Windows (XP NTFS). -- v3: advpack: DelNode support ADN_DEL_IF_EMPTY flag https://gitlab.winehq.org/wine/wine/-/merge_requests/6347
From: anders <andersdev(a)proton.me> --- dlls/advpack/files.c | 10 ++++++++-- dlls/advpack/tests/advpack.c | 9 ++++++++- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/dlls/advpack/files.c b/dlls/advpack/files.c index 2bf2481cca4..fa247cb3f07 100644 --- a/dlls/advpack/files.c +++ b/dlls/advpack/files.c @@ -320,6 +320,9 @@ static HRESULT DELNODE_recurse_dirtree(LPWSTR fname, DWORD flags) BOOL done = TRUE; int fname_len = lstrlenW(fname); + if (flags & ADN_DEL_IF_EMPTY) + goto onlyemptydir; + /* Generate a path with wildcard suitable for iterating */ if (fname_len && fname[fname_len-1] != '\\') fname[fname_len++] = '\\'; lstrcpyW(fname + fname_len, L"*"); @@ -347,8 +350,10 @@ static HRESULT DELNODE_recurse_dirtree(LPWSTR fname, DWORD flags) if (done) { +onlyemptydir: TRACE("%s: directory\n", debugstr_w(fname)); - if (SetFileAttributesW(fname, FILE_ATTRIBUTE_NORMAL) && RemoveDirectoryW(fname)) + SetFileAttributesW(fname, FILE_ATTRIBUTE_NORMAL); + if (RemoveDirectoryW(fname)) { ret = S_OK; } @@ -357,7 +362,8 @@ static HRESULT DELNODE_recurse_dirtree(LPWSTR fname, DWORD flags) else { TRACE("%s: file\n", debugstr_w(fname)); - if (SetFileAttributesW(fname, FILE_ATTRIBUTE_NORMAL) && DeleteFileW(fname)) + SetFileAttributesW(fname, FILE_ATTRIBUTE_NORMAL); + if (DeleteFileW(fname)) { ret = S_OK; } diff --git a/dlls/advpack/tests/advpack.c b/dlls/advpack/tests/advpack.c index 33d5f4bc52b..2e49c332973 100644 --- a/dlls/advpack/tests/advpack.c +++ b/dlls/advpack/tests/advpack.c @@ -160,6 +160,10 @@ static void delnode_test(void) hr = pDelNode(lstrcatA(currDir, "\\DelNodeTestDir"), 0); ok (hr == S_OK, "DelNode failed deleting an empty directory\n"); currDir[currDirLen] = '\0'; + CreateDirectoryA("DelNodeTestDir", NULL); + hr = pDelNode(lstrcatA(currDir, "\\DelNodeTestDir"), ADN_DEL_IF_EMPTY); + ok (hr == S_OK, "DelNode ADN_DEL_IF_EMPTY failed deleting an empty directory\n"); + currDir[currDirLen] = '\0'; /* Test deletion of a directory containing one file. */ CreateDirectoryA("DelNodeTestDir", NULL); @@ -167,7 +171,10 @@ static void delnode_test(void) CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL); assert(hn != INVALID_HANDLE_VALUE); CloseHandle(hn); - hr = pDelNode(lstrcatA(currDir, "\\DelNodeTestDir"), 0); + lstrcatA(currDir, "\\DelNodeTestDir"); + hr = pDelNode(currDir, ADN_DEL_IF_EMPTY); + ok (FAILED(hr), "DelNode ADN_DEL_IF_EMPTY should not delete non-empty\n"); + hr = pDelNode(currDir, 0); ok (hr == S_OK, "DelNode failed deleting a directory containing one file\n"); currDir[currDirLen] = '\0'; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6347
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=147963 Your paranoid android. === debian11b (64 bit WoW report) === kernel32: comm.c:1586: Test failed: Unexpected time 1000, expected around 500
On Fri Aug 23 20:48:29 2024 +0000, Elizabeth Figura wrote:
That's not a very meaningful test; the point of the flag is to prevent non-empty trees from being deleted, but you're not testing that case. I'm testing both empty and non-empty with the flag.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6347#note_79818
On Fri Aug 23 20:20:46 2024 +0000, Elizabeth Figura wrote:
That's a separate and unrelated change; it should be in its own commit and supported by tests. That would be a lot of ACL code to test that. I'd rather just rip out this change if that is what people want.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6347#note_79819
On Fri Aug 23 20:20:10 2024 +0000, Elizabeth Figura wrote:
Surely this can be done without a goto. It can if I copy the code at the goto location but is that somehow better? More code just to avoid the goto?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6347#note_79820
On Fri Aug 23 20:52:08 2024 +0000, Anders Kjersem wrote:
It can if I copy the code at the goto location but is that somehow better? More code just to avoid the goto? Or you can simply move the label one line higher (done is always TRUE at that point, so that changes nothing), then convert it to a simple if. Sure, it adds another level of indentation to ~25 lines, but who cares.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6347#note_79821
On Fri Aug 23 20:50:39 2024 +0000, Anders Kjersem wrote:
That would be a lot of ACL code to test that. I'd rather just rip out this change if that is what people want. What was the point of adding it in the first place?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6347#note_79893
On Fri Aug 23 20:48:29 2024 +0000, Anders Kjersem wrote:
I'm testing both empty and non-empty with the flag. My bad, I misread.
That said I would test for a specific error code instead of FAILED(hr). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6347#note_79894
On Fri Aug 23 21:25:53 2024 +0000, Elizabeth Figura wrote:
What was the point of adding it in the first place? Because the `SetFileAttributes` call is not super important, it's there to remove the READONLY attribute that might be present. But it can also block `RemoveDirectory`/`DeleteFile` from working if the user only has DELETE rights and not FILE_WRITE_ATTRIBUTES.
The actual use case for this (on Windows) is deleting a shortcut on the %allusersprofile% desktop. Some installers will create a shortcut there with DELETE access allowed for the everyone group. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6347#note_79895
On Fri Aug 23 21:26:21 2024 +0000, Elizabeth Figura wrote:
My bad, I misread. That said I would test for a specific error code instead of FAILED(hr). All error paths return E_FAIL yes but I don't know if that is guaranteed under Windows (it did when I tested but that is no guarantee).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6347#note_79896
On Sat Aug 24 00:41:18 2024 +0000, Anders Kjersem wrote:
All error paths return E_FAIL yes but I don't know if that is guaranteed under Windows (it did when I tested but that is no guarantee). It's still worth having as specific knowledge as we can. If it ever changes in a Windows version we'll notice and adjust the tests.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6347#note_79930
participants (5)
-
Alfred Agrell (＠Alcaro) -
anders
-
Anders Kjersem (＠anders)
-
Elizabeth Figura (＠zfigura)
-
Marvin