[PATCH v2 0/3] MR6333: odbccp32 valid dsn

[Alistair Leslie-Hughes (＠alesliehughes)]

--
  v2: odbccp32: SQLWrite/RemoveDSNFromIniW correctly handle config_mode
      odbccp32: SQLRemoveDSNFromIniW check for valid DSN before delete
      odbccp32: SQLValidDSNW return false on empty string.

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

[PATCH v2 1/3] odbccp32: SQLValidDSNW return false on empty string.

[Alistair Leslie-Hughes]

From: Alistair Leslie-Hughes

---
 dlls/odbccp32/odbccp32.c   | 4 ++--
 dlls/odbccp32/tests/misc.c | 6 ++++++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/dlls/odbccp32/odbccp32.c b/dlls/odbccp32/odbccp32.c
index dba24e4b673..ab745ef1bb7 100644
--- a/dlls/odbccp32/odbccp32.c
+++ b/dlls/odbccp32/odbccp32.c
@@ -1725,7 +1725,7 @@ BOOL WINAPI SQLValidDSNW(LPCWSTR lpszDSN)
     clear_errors();
     TRACE("%s\n", debugstr_w(lpszDSN));

-    if (!lpszDSN || lstrlenW(lpszDSN) > SQL_MAX_DSN_LENGTH || wcspbrk(lpszDSN, L"[]{}(),;?*=!@\"))
+    if (!lpszDSN || !*lpszDSN || lstrlenW(lpszDSN) > SQL_MAX_DSN_LENGTH || wcspbrk(lpszDSN, L"[]{}(),;?*=!@\"))
     {
         return FALSE;
     }
@@ -1739,7 +1739,7 @@ BOOL WINAPI SQLValidDSN(LPCSTR lpszDSN)
     clear_errors();
     TRACE("%s\n", debugstr_a(lpszDSN));

-    if (!lpszDSN || strlen(lpszDSN) > SQL_MAX_DSN_LENGTH || strpbrk(lpszDSN, invalid))
+    if (!lpszDSN || !*lpszDSN || strlen(lpszDSN) > SQL_MAX_DSN_LENGTH || strpbrk(lpszDSN, invalid))
     {
         return FALSE;
     }
diff --git a/dlls/odbccp32/tests/misc.c b/dlls/odbccp32/tests/misc.c
index e9f879edda6..b94ed811337 100644
--- a/dlls/odbccp32/tests/misc.c
+++ b/dlls/odbccp32/tests/misc.c
@@ -816,6 +816,9 @@ static void test_SQLValidDSN(void)
     /* Max DSN name value */
     ret = SQLValidDSN("12345678901234567890123456789012");
     ok(ret, "got %d\n", ret);
+
+    ret = SQLValidDSN("");
+    ok(!ret, "got %d\n", ret);
 }

static void test_SQLValidDSNW(void)
@@ -843,6 +846,9 @@ static void test_SQLValidDSNW(void)

ret = SQLValidDSNW(L"12345678901234567890123456789012");
     ok(ret, "got %d\n", ret);
+
+    ret = SQLValidDSNW(L"");
+    ok(!ret, "got %d\n", ret);
 }

static void test_SQLConfigDataSource(void)

[PATCH v2 2/3] odbccp32: SQLRemoveDSNFromIniW check for valid DSN before delete

[Alistair Leslie-Hughes]

From: Alistair Leslie-Hughes

---
 dlls/odbccp32/odbccp32.c   | 6 ++++++
 dlls/odbccp32/tests/misc.c | 6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/dlls/odbccp32/odbccp32.c b/dlls/odbccp32/odbccp32.c
index ab745ef1bb7..b77c5698c07 100644
--- a/dlls/odbccp32/odbccp32.c
+++ b/dlls/odbccp32/odbccp32.c
@@ -1586,6 +1586,12 @@ BOOL WINAPI SQLRemoveDSNFromIniW(LPCWSTR lpszDSN)

TRACE("%s\n", debugstr_w(lpszDSN));

+    if (!SQLValidDSNW(lpszDSN))
+    {
+        push_error(ODBC_ERROR_INVALID_DSN, L"Invalid DSN");
+        return FALSE;
+    }
+
     clear_errors();

if (RegOpenKeyW(HKEY_LOCAL_MACHINE, L"Software\ODBC\ODBC.INI\ODBC Data Sources", &hkey) == ERROR_SUCCESS)
diff --git a/dlls/odbccp32/tests/misc.c b/dlls/odbccp32/tests/misc.c
index b94ed811337..af149e6c973 100644
--- a/dlls/odbccp32/tests/misc.c
+++ b/dlls/odbccp32/tests/misc.c
@@ -885,6 +885,12 @@ static void test_SQLWriteDSNToIni(void)

SQLSetConfigMode(ODBC_SYSTEM_DSN);

+    ret = SQLRemoveDSNFromIni("");
+    ok(!ret, "got %d\n", ret);
+
+    ret = SQLRemoveDSNFromIniW(L"");
+    ok(!ret, "got %d\n", ret);
+
     ret = SQLWriteDSNToIni("wine_dbs", "SQL Server");
     if (!ret)
     {

[PATCH v2 3/3] odbccp32: SQLWrite/RemoveDSNFromIniW correctly handle config_mode

[Alistair Leslie-Hughes]

From: Alistair Leslie-Hughes

---
 dlls/odbccp32/odbccp32.c   | 28 +++++++++++++++++++++++-----
 dlls/odbccp32/tests/misc.c | 14 ++++++++++++++
 2 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/dlls/odbccp32/odbccp32.c b/dlls/odbccp32/odbccp32.c
index b77c5698c07..e7ba731bb3e 100644
--- a/dlls/odbccp32/odbccp32.c
+++ b/dlls/odbccp32/odbccp32.c
@@ -1582,7 +1582,7 @@ BOOL WINAPI SQLRemoveDriverManager(LPDWORD pdwUsageCount)

BOOL WINAPI SQLRemoveDSNFromIniW(LPCWSTR lpszDSN)
 {
-    HKEY hkey;
+    HKEY hkey, hkeyroot = HKEY_CURRENT_USER;

TRACE("%s\n", debugstr_w(lpszDSN));

@@ -1594,13 +1594,28 @@ BOOL WINAPI SQLRemoveDSNFromIniW(LPCWSTR lpszDSN)

clear_errors();

-    if (RegOpenKeyW(HKEY_LOCAL_MACHINE, L"Software\ODBC\ODBC.INI\ODBC Data Sources", &hkey) == ERROR_SUCCESS)
+    if (config_mode == ODBC_SYSTEM_DSN)
+        hkeyroot = HKEY_LOCAL_MACHINE;
+    else if (config_mode == ODBC_BOTH_DSN)
+    {
+        WCHAR *regpath = malloc( (wcslen(L"Software\ODBC\ODBC.INI\ODBC Data Sources\") + wcslen(lpszDSN) + 1) * sizeof(WCHAR) );
+        /* ONLY removes one DSN, USER or SYSTEM */
+        if (RegOpenKeyW(HKEY_CURRENT_USER, L"Software\ODBC\ODBC.INI\ODBC Data Sources", &hkey) == ERROR_SUCCESS)
+            hkeyroot = HKEY_CURRENT_USER;
+        else
+            hkeyroot = HKEY_LOCAL_MACHINE;
+
+        RegCloseKey(hkey);
+        free(regpath);
+    }
+
+    if (RegOpenKeyW(hkeyroot, L"Software\ODBC\ODBC.INI\ODBC Data Sources", &hkey) == ERROR_SUCCESS)
     {
         RegDeleteValueW(hkey, lpszDSN);
         RegCloseKey(hkey);
     }

-    if (RegOpenKeyW(HKEY_LOCAL_MACHINE, L"Software\ODBC\ODBC.INI", &hkey) == ERROR_SUCCESS)
+    if (RegOpenKeyW(hkeyroot, L"Software\ODBC\ODBC.INI", &hkey) == ERROR_SUCCESS)
     {
         RegDeleteTreeW(hkey, lpszDSN);
         RegCloseKey(hkey);
@@ -1756,7 +1771,7 @@ BOOL WINAPI SQLValidDSN(LPCSTR lpszDSN)
 BOOL WINAPI SQLWriteDSNToIniW(LPCWSTR lpszDSN, LPCWSTR lpszDriver)
 {
     DWORD ret;
-    HKEY hkey, hkeydriver;
+    HKEY hkey, hkeydriver, hkeyroot = HKEY_LOCAL_MACHINE;
     WCHAR filename[MAX_PATH];

TRACE("%s %s\n", debugstr_w(lpszDSN), debugstr_w(lpszDriver));
@@ -1784,7 +1799,10 @@ BOOL WINAPI SQLWriteDSNToIniW(LPCWSTR lpszDSN, LPCWSTR lpszDriver)
         RegCloseKey(hkey);
     }

-    if ((ret = RegCreateKeyW(HKEY_LOCAL_MACHINE, L"SOFTWARE\ODBC\ODBC.INI", &hkey)) == ERROR_SUCCESS)
+    if (config_mode == ODBC_USER_DSN)
+        hkeyroot = HKEY_CURRENT_USER;
+
+    if ((ret = RegCreateKeyW(hkeyroot, L"SOFTWARE\ODBC\ODBC.INI", &hkey)) == ERROR_SUCCESS)
     {
         HKEY sources;

diff --git a/dlls/odbccp32/tests/misc.c b/dlls/odbccp32/tests/misc.c
index af149e6c973..c2ad7340fd9 100644
--- a/dlls/odbccp32/tests/misc.c
+++ b/dlls/odbccp32/tests/misc.c
@@ -898,6 +898,12 @@ static void test_SQLWriteDSNToIni(void)
         return;
     }

+    SQLSetConfigMode(ODBC_USER_DSN);
+    ret = SQLWriteDSNToIni("wine_dbs", "SQL Server");
+    ok(ret, "got %d\n", ret);
+
+    SQLSetConfigMode(ODBC_SYSTEM_DSN);
+
     if(ret)
     {
         HKEY hkey;
@@ -920,6 +926,12 @@ static void test_SQLWriteDSNToIni(void)
             RegCloseKey(hkey);
         }

+        SQLSetConfigMode(ODBC_BOTH_DSN);
+
+        /* ODBC_BOTH_DSN set and has both System/User DSN but only removes USER. */
+        ret = SQLRemoveDSNFromIni("wine_dbs");
+        ok(ret, "got %d\n", ret);
+
         res = RegOpenKeyExA(HKEY_LOCAL_MACHINE, "Software\ODBC\ODBC.INI\wine_dbs", 0,
                             KEY_READ, &hkey);
         ok(res == ERROR_SUCCESS, "RegOpenKeyExW failed\n");
@@ -938,6 +950,8 @@ static void test_SQLWriteDSNToIni(void)
             RegCloseKey(hkey);
         }

+        SQLSetConfigMode(ODBC_SYSTEM_DSN);
+
         ret = SQLRemoveDSNFromIni("wine_dbs");
         ok(ret, "got %d\n", ret);
     }

[Hans Leidekker (＠hans)]

Hans Leidekker (@hans) commented about dlls/odbccp32/odbccp32.c:
>  clear_errors();
>
> - if (RegOpenKeyW(HKEY_LOCAL_MACHINE, L"Software\ODBC\ODBC.INI\ODBC Data Sources", &hkey) == ERROR_SUCCESS)
> - if (config_mode == ODBC_SYSTEM_DSN)
> -
>    hkeyroot = HKEY_LOCAL_MACHINE;
> - else if (config_mode == ODBC_BOTH_DSN)
> - {
> -
>    WCHAR *regpath = malloc( (wcslen(L"Software\\ODBC\\ODBC.INI\\ODBC Data Sources\\") + wcslen(lpszDSN) + 1) * sizeof(WCHAR) );
> -
>    /* ONLY removes one DSN, USER or SYSTEM */
> -
>    if (RegOpenKeyW(HKEY_CURRENT_USER, L"Software\\ODBC\\ODBC.INI\\ODBC Data Sources", &hkey) == ERROR_SUCCESS)
> -
>        hkeyroot = HKEY_CURRENT_USER;
> -
>    else
> -
>        hkeyroot = HKEY_LOCAL_MACHINE;
> -
> -
>    RegCloseKey(hkey);
> -
>    free(regpath);

I guess you meant to build regpath and pass it to RegOpenKeyW(). Please add registry checks to the tests.