[PATCH v2 0/1] MR252: shell32/tests: Add tests for Program Manager name sanitization.

[Alex Henrie (＠alexhenrie)]

Split from https://gitlab.winehq.org/wine/wine/-/merge_requests/23

--
  v2: shell32/tests: Add tests for Program Manager name sanitization.

https://gitlab.winehq.org/wine/wine/-/merge_requests/252

[PATCH v2 1/1] shell32/tests: Add tests for Program Manager name sanitization.

[Alex Henrie]

From: Alex Henrie

Signed-off-by: Alex Henrie 
---
 dlls/shell32/tests/progman_dde.c | 156 ++++++++++++++++++++++++++++++-
 1 file changed, 155 insertions(+), 1 deletion(-)

diff --git a/dlls/shell32/tests/progman_dde.c b/dlls/shell32/tests/progman_dde.c
index 5d532f9222f..57d8d0ba1f7 100644
--- a/dlls/shell32/tests/progman_dde.c
+++ b/dlls/shell32/tests/progman_dde.c
@@ -23,7 +23,7 @@
  *   functionality
  * - Todo: Handle CommonGroupFlag
  *         Better AddItem Tests (Lots of parameters to test)
- *         Tests for Invalid Characters in Names / Invalid Parameters
+ *         Tests for invalid parameters
  */

#include <stdio.h>
@@ -425,6 +425,159 @@ static void test_request_groups(DWORD instance, HCONV hconv)
     FindClose(hfind);
 }

+static BOOL is_unsanitary(char c)
+{
+    return (c > 0 && c < ' ') || strchr("*/:<>?\|", c) != NULL;
+}
+
+static void sanitize_name(const char *original_name, char *sanitized_name, BOOL group)
+{
+    BOOL at_end = TRUE;
+    int i;
+
+    i = strlen(original_name);
+    sanitized_name[i] = 0;
+
+    while (--i >= 0)
+    {
+        if (is_unsanitary(original_name[i]))
+        {
+            /* replaced in all positions */
+            sanitized_name[i] = '_';
+            at_end = FALSE;
+        }
+        else if (original_name[i] == '.' || (original_name[i] == ' ' && group))
+        {
+            /* left alone if in the middle of the string, dropped if at the end of the string */
+            sanitized_name[i] = at_end ? '\0' : original_name[i];
+        }
+        else
+        {
+            /* left alone in all positions */
+            sanitized_name[i] = original_name[i];
+            at_end = FALSE;
+        }
+    }
+}
+
+static void test_name_sanitization(DWORD instance, HCONV hConv)
+{
+    static const char test_chars[] = "\x01\x1F !#$%&'*+,-./;<=>?@[\]^`{|}~\x7F\xFF";
+    char original_name[16], sanitized_icon_name[16], sanitized_group_name[16];
+    char buf[64];
+    UINT error;
+    int i;
+    char c;
+
+    if (0) /* the directory isn't deleted on windows < 7 */
+    {
+        error = dde_execute(instance, hConv, "[CreateGroup(" ")]");
+        ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error);
+        ok(check_exists(" "), "directory not created\n");
+        ok(!check_window_exists(" "), "window should not exist\n");
+
+        error = dde_execute(instance, hConv, "[DeleteGroup(" ")]");
+        ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error);
+        ok(!check_exists(" "), "directory should not exist\n");
+    }
+
+    if (0) /* these calls will actually delete the start menu */
+    {
+        error = dde_execute(instance, hConv, "[DeleteGroup("")]");
+        ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error);
+        ok(!check_exists("../Programs"), "directory should not exist\n");
+
+        error = dde_execute(instance, hConv, "[DeleteGroup("..")]");
+        ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error);
+        ok(!check_exists("../../Start Menu"), "directory should not exist\n");
+    }
+
+    for (i = 0; i < sizeof(test_chars) - 1; i++)
+    {
+        c = test_chars[i];
+        winetest_push_context("char %d '%c'", c, c);
+
+        sprintf(original_name, "%03d_%c_.%c", c, c, c);
+        sanitize_name(original_name, sanitized_group_name, TRUE);
+        sanitize_name(original_name, sanitized_icon_name, FALSE);
+
+        sprintf(buf, "[CreateGroup("Group%s")]", original_name);
+        error = dde_execute(instance, hConv, buf);
+        ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error);
+        sprintf(buf, "Group%s", sanitized_group_name);
+        todo_wine_if(is_unsanitary(c)) ok(check_exists(buf), "directory not created\n");
+        if (!check_exists(buf)) continue;
+        todo_wine_if(is_unsanitary(c)) ok(check_window_exists(buf) || broken(c == ' ') /* vista */, "window not created\n");
+
+        sprintf(buf, "[ShowGroup("Group%s", 0)]", original_name);
+        error = dde_execute(instance, hConv, buf);
+        ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error);
+        sprintf(buf, "Group%s", sanitized_group_name);
+        todo_wine_if(is_unsanitary(c)) ok(check_window_exists(buf) || broken(c == ' ') /* vista */, "window not created\n");
+
+        if (c == ' ')
+        {
+            /* Although no error is reported, no icon is created if the group name ends in a space */
+            error = dde_execute(instance, hConv, "[AddItem(notepad,Notepad)]");
+            todo_wine ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error);
+            sprintf(buf, "Group%s/Notepad.lnk", sanitized_group_name);
+            ok(!check_exists(buf), "link should not exist\n");
+            goto delete_group;
+        }
+
+        sprintf(buf, "[AddItem(notepad,"Notepad%s")]", original_name);
+        error = dde_execute(instance, hConv, buf);
+        todo_wine_if(is_unsanitary(c) || c == '.') ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error);
+        sprintf(buf, "Group%s/Notepad%s.lnk", sanitized_group_name, sanitized_icon_name);
+        todo_wine_if(is_unsanitary(c) || c == '.') ok(check_exists(buf) || broken(c == '.') /* XP */, "link not created\n");
+        if (!check_exists(buf)) goto delete_group;
+
+        if (is_unsanitary(c))
+        {
+            sprintf(buf, "[ReplaceItem("Notepad%s")]", original_name);
+            error = dde_execute(instance, hConv, buf);
+            ok(error == DMLERR_NOTPROCESSED, "expected DMLERR_NOTPROCESSED, got %#x\n", error);
+            todo_wine sprintf(buf, "Group%s/Notepad%s.lnk", sanitized_group_name, sanitized_icon_name);
+            ok(check_exists(buf), "link should still exist\n");
+
+            sprintf(buf, "[DeleteItem("Notepad%s")]", original_name);
+            error = dde_execute(instance, hConv, buf);
+            ok(error == DMLERR_NOTPROCESSED, "expected DMLERR_NOTPROCESSED, got %#x\n", error);
+            todo_wine sprintf(buf, "Group%s/Notepad%s.lnk", sanitized_group_name, sanitized_icon_name);
+            ok(check_exists(buf), "link should still exist\n");
+        }
+        else
+        {
+            sprintf(buf, "[ReplaceItem("Notepad%s")]", original_name);
+            error = dde_execute(instance, hConv, buf);
+            ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error);
+            sprintf(buf, "Group%s/Notepad%s.lnk", sanitized_group_name, sanitized_icon_name);
+            ok(!check_exists(buf), "link should not exist\n");
+
+            sprintf(buf, "[AddItem(notepad,"Notepad%s")]", original_name);
+            error = dde_execute(instance, hConv, buf);
+            ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error);
+            sprintf(buf, "Group%s/Notepad%s.lnk", sanitized_group_name, sanitized_icon_name);
+            ok(check_exists(buf), "link not created\n");
+
+            sprintf(buf, "[DeleteItem("Notepad%s")]", original_name);
+            error = dde_execute(instance, hConv, buf);
+            ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error);
+            sprintf(buf, "Group%s/Notepad%s.lnk", sanitized_group_name, sanitized_icon_name);
+            ok(!check_exists(buf), "link should not exist\n");
+        }
+
+delete_group:
+        sprintf(buf, "[DeleteGroup("Group%s")]", original_name);
+        error = dde_execute(instance, hConv, buf);
+        todo_wine_if(is_unsanitary(c) && strchr("*:?", c) == NULL) ok(error == DMLERR_NO_ERROR, "expected DMLERR_NO_ERROR, got %#x\n", error);
+        sprintf(buf, "Group%s", sanitized_group_name);
+        ok(!check_exists(buf), "directory should not exist\n");
+
+        winetest_pop_context();
+    }
+}
+
 START_TEST(progman_dde)
 {
     DWORD instance = 0;
@@ -479,6 +632,7 @@ START_TEST(progman_dde)

/* Run Tests */
     test_progman_dde2(instance, hConv);
+    test_name_sanitization(instance, hConv);

/* Cleanup & Exit */
     ret = DdeDisconnect(hConv);

[PATCH v2 1/1] shell32/tests: Add tests for Program Manager name sanitization.

[Marvin]

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 full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=117158

Your paranoid android.

=== w10pro64_ja (64 bit report) ===

shell32:
progman_dde.c:573: Test failed: char 39 ''': expected DMLERR_NO_ERROR, got 0x4005
progman_dde.c:575: Test failed: char 39 ''': directory should not exist
progman_dde.c:506: Test failed: char 42 '*': expected DMLERR_NO_ERROR, got 0x4006
progman_dde.c:508: Test failed: char 42 '*': directory not created
progman_dde.c:506: Test failed: char 42 '*': char 43 '+': expected DMLERR_NO_ERROR, got 0x4006
progman_dde.c:508: Test failed: char 42 '*': char 43 '+': directory not created
progman_dde.c:506: Test failed: char 42 '*': char 43 '+': char 44 ',': expected DMLERR_NO_ERROR, got 0x4006
progman_dde.c:508: Test failed: char 42 '*': char 43 '+': char 44 ',': directory not created
progman_dde.c:506: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': expected DMLERR_NO_ERROR, got 0x4006
progman_dde.c:508: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': directory not created
progman_dde.c:506: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': expected DMLERR_NO_ERROR, got 0x4006
progman_dde.c:508: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': directory not created
progman_dde.c:506: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': expected DMLERR_NO_ERROR, got 0x4006
progman_dde.c:508: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': directory not created
progman_dde.c:506: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': char 59 ';': expected DMLERR_NO_ERROR, got 0x4006
progman_dde.c:508: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': char 59 ';': directory not created
progman_dde.c:506: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': char 59 ';': char 60 '<': expected DMLERR_NO_ERROR, got 0x4006
progman_dde.c:508: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': char 59 ';': char 60 '<': directory not created
progman_dde.c:506: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': char 59 ';': char 60 '<': 	: expected DMLERR_NO_ERROR, got 0x4006
progman_dde.c:508: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': char 59 ';': char 60 '<': 	: directory not created
progman_dde.c:506: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': char 59 ';': char 60 '<': 
progman_dde.c:508: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': char 59 ';': char 60 '<': 
progman_dde.c:506: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': char 59 ';': char 60 '<': : : : expected DMLERR_NO_ERROR, got 0x4006
progman_dde.c:508: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': char 59 ';': char 60 '<': : : : directory not created
progman_dde.c:506: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': char 59 ';': char 60 '<': : : : : expected DMLERR_NO_ERROR, got 0x4006
progman_dde.c:508: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': char 59 ';': char 60 '<': : : : : directory not created
progman_dde.c:506: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': char 59 ';': char 60 '<': 
: : : : : expected DMLERR_NO_ERROR, got 0x4006
progman_dde.c:508: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': char 59 ';': char 60 '<': 
: : : : : directory not created
progman_dde.c:506: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': char 59 ';': char 60 '<': : : : : : : expected DMLERR_NO_ERROR, got 0x4006
progman_dde.c:508: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': char 59 ';': char 60 '<': : : : : : : directory not created
progman_dde.c:506: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': char 59 ';': char 60 '<': : : : : : : : expected DMLERR_NO_ERROR, got 0x4006
progman_dde.c:508: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': char 59 ';': char 60 '<': : : : : : : : directory not created
progman_dde.c:506: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': char 59 ';': char 60 '<': : : : : : : : : expected DMLERR_NO_ERROR, got 0x4006
progman_dde.c:508: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': char 59 ';': char 60 '<': : : : : : : : : directory not created
progman_dde.c:506: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': char 59 ';': char 60 '<': : : : : : : : : T

: expected DMLERR_NO_ERROR, got 0x4006
progman_dde.c:508: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': char 59 ';': char 60 '<': : : : : : : : : T

: directory not created
progman_dde.c:506: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': char 59 ';': char 60 '<': : : : : : : : : T

: °: expected DMLERR_NO_ERROR, got 0x4006
progman_dde.c:508: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': char 59 ';': char 60 '<': : : : : : : : : T

: °: directory not created
progman_dde.c:506: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': char 59 ';': char 60 '<': : : : : : : : : T

: °: Ò

: expected DMLERR_NO_ERROR, got 0x4006
progman_dde.c:508: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': char 59 ';': char 60 '<': : : : : : : : : T

: °: Ò

: directory not created
progman_dde.c:506: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': char 59 ';': char 60 '<': : : : : : : : : T

: °: Ò

: D

: expected DMLERR_NO_ERROR, got 0x4006
progman_dde.c:508: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': char 59 ';': char 60 '<': : : : : : : : : T

: °: Ò

: D

: directory not created
progman_dde.c:506: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': char 59 ';': char 60 '<': : : : : : : : : T

: °: Ò

: D

: Š
: expected DMLERR_NO_ERROR, got 0x4006
progman_dde.c:508: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': char 59 ';': char 60 '<': : : : : : : : : T

: °: Ò

: D

: Š
: directory not created
progman_dde.c:506: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': char 59 ';': char 60 '<': : : : : : : : : T

: °: Ò

: D

: Š
: : expected DMLERR_NO_ERROR, got 0x4006
progman_dde.c:508: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': char 59 ';': char 60 '<': : : : : : : : : T

: °: Ò

: D

: Š
: : directory not created
progman_dde.c:506: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': char 59 ';': char 60 '<': : : : : : : : : T

: °: Ò

: D

: Š
: : : expected DMLERR_NO_ERROR, got 0x4006
progman_dde.c:508: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': char 59 ';': char 60 '<': : : : : : : : : T

: °: Ò

: D

: Š
: : : directory not created
progman_dde.c:639: Test failed: char 42 '*': char 43 '+': char 44 ',': char 45 '-': char 46 '.': char 47 '/': char 59 ';': char 60 '<': : : : : : : : : T

: °: Ò

: D

: Š
: : : DdeDisconnect() failed: 16390

[Alex Henrie (＠alexhenrie)]

I benchmarked it today and on my laptop, the new tests raised the test time from 5 seconds to 2 minutes 15 seconds. I can see how that would be a problem.

I've now pared the tests back to the bare minimum set of tests that I believe are necessary to demonstrate that my proposed solution in merge request !23 solves the problem. Paring back the tests dropped the test time to 36 seconds, mainly because the check_window_exists tests are now skipped if check_exists has already failed. (check_window_exists is very fast if the window does exist, but it will spend a long time waiting for a window that never appears.)

Would it be acceptable to start by committing this set of tests, and build on it later as needed?

[Alexandre Julliard (＠julliard)]

36 seconds is still a lot for such an obscure feature. I don't think you need to test every char one by one.