[PATCH 1/2 v2] odbccp32: Implement SQLInstallDriverEx correctly
Sebastian Lackner
sebastian at fds-team.de
Tue Feb 9 02:53:27 CST 2016
Sorry for the late reply, didn't see that this patch was assigned to me.
Below is some feedback on this iteration.
On 07.01.2016 08:15, Alistair Leslie-Hughes wrote:
> Signed-off-by: Alistair Leslie-Hughes <leslie_alistair at hotmail.com>
> ---
> dlls/odbccp32/odbccp32.c | 90 +++++++++++++++++++++++++++++++++++++++++-----
> dlls/odbccp32/tests/misc.c | 59 ++++++++++++++++++++++++++++++
> 2 files changed, 140 insertions(+), 9 deletions(-)
>
> diff --git a/dlls/odbccp32/odbccp32.c b/dlls/odbccp32/odbccp32.c
> index bc643d4..3d150a1 100644
> --- a/dlls/odbccp32/odbccp32.c
> +++ b/dlls/odbccp32/odbccp32.c
> @@ -29,6 +29,7 @@
> #include "winbase.h"
> #include "winreg.h"
> #include "winnls.h"
> +#include "wine/unicode.h"
> #include "wine/debug.h"
>
> #include "odbcinst.h"
> @@ -38,6 +39,8 @@ WINE_DEFAULT_DEBUG_CHANNEL(odbc);
> /* Registry key names */
> static const WCHAR drivers_key[] = {'S','o','f','t','w','a','r','e','\\','O','D','B','C','\\','O','D','B','C','I','N','S','T','.','I','N','I','\\','O','D','B','C',' ','D','r','i','v','e','r','s',0};
> static const WCHAR odbcW[] = {'S','o','f','t','w','a','r','e','\\','O','D','B','C',0};
> +static const WCHAR odbcini[] = {'S','o','f','t','w','a','r','e','\\','O','D','B','C','\\','O','D','B','C','I','N','S','T','.','I','N','I','\\',0};
> +static const WCHAR odbcdrivers[] = {'O','D','B','C',' ','D','r','i','v','e','r','s',0};
>
> /* This config mode is known to be process-wide.
> * MSDN documentation suggests that the value is hidden somewhere in the registry but I haven't found it yet.
> @@ -626,12 +629,85 @@ BOOL WINAPI SQLInstallDriver(LPCSTR lpszInfFile, LPCSTR lpszDriver,
> pcbPathOut, ODBC_INSTALL_COMPLETE, &usage);
> }
>
> +static void write_registry_values(const WCHAR *regkey, const WCHAR *driver, const WCHAR *path_in, WCHAR *path)
> +{
> + static const WCHAR installed[] = {'I','n','s','t','a','l','l','e','d',0};
> + static const WCHAR slash[] = {'\\', 0};
> + static const WCHAR driverW[] = {'D','r','i','v','e','r',0};
> + static const WCHAR setupW[] = {'S','e','t','u','p',0};
> + HKEY hkey, hkeydriver;
> +
> + if (RegCreateKeyW(HKEY_LOCAL_MACHINE, odbcini, &hkey) == ERROR_SUCCESS)
> + {
> + if (RegCreateKeyW(hkey, regkey, &hkeydriver) == ERROR_SUCCESS)
> + {
> + if(RegSetValueExW(hkeydriver, driver, 0, REG_SZ, (BYTE*)installed,
> + (lstrlenW(installed)+1)*sizeof(WCHAR)) != ERROR_SUCCESS)
> + ERR("Failed to write registry installed key\n");
I think it would be better to let SQLInstallDriverExW fail when things go wrong.
> +
> + RegCloseKey(hkeydriver);
> + }
> +
> + if (RegCreateKeyW(hkey, driver, &hkeydriver) == ERROR_SUCCESS)
> + {
> + WCHAR entry[1024];
> + WCHAR value[MAX_PATH];
> + const WCHAR *p;
> +
> + /* Skip name entry */
> + p = driver;
> + p += lstrlenW(p) + 1;
> +
> + if(!path_in)
> + GetSystemDirectoryW(path, MAX_PATH);
> + else
> + lstrcpyW(path, path_in);
If one of the registry operations above fails, path will be unset, without
propagating this failure to the parent function.
> +
> + for (; *p; p += lstrlenW(p) + 1)
> + {
> + WCHAR *divider = strchrW(p,'=');
> +
> + if (divider)
> + {
> + /* Write pair values to the registry. */
> + lstrcpynW(entry, p, divider - p + 1);
The size limit for entry / value could lead to buffer overflows. I'm aware that wine
already contains a lot of similar issues, but usually it is recommended to avoid such
design decisions for new code (unless Windows has the same limitiation).
> +
> + divider++;
> + TRACE("Writing pair %s,%s\n", debugstr_w(entry), debugstr_w(divider));
> +
> + /* Driver and Setup entries use the system path unless a path is specified. */
> + if(lstrcmpiW(driverW, entry) == 0 || lstrcmpiW(setupW, entry) == 0)
> + {
> + lstrcpyW(value, path);
> + lstrcatW(value, slash);
> + lstrcatW(value, divider);
> + }
> + else
> + lstrcpyW(value, divider);
> +
> + if(RegSetValueExW(hkeydriver, entry, 0, REG_SZ, (BYTE*)value,
> + (lstrlenW(value)+1)*sizeof(WCHAR)) != ERROR_SUCCESS)
> + ERR("Failed to write registry data %s %s\n", debugstr_w(entry), debugstr_w(value));
> + }
> + else
> + {
> + ERR("No pair found. %s\n", debugstr_w(p));
> + break;
> + }
> + }
> +
> + RegCloseKey(hkeydriver);
> + }
> +
> + RegCloseKey(hkey);
> + }
> +}
> +
> BOOL WINAPI SQLInstallDriverExW(LPCWSTR lpszDriver, LPCWSTR lpszPathIn,
> LPWSTR lpszPathOut, WORD cbPathOutMax, WORD *pcbPathOut,
> WORD fRequest, LPDWORD lpdwUsageCount)
> {
> UINT len;
> - LPCWSTR p;
> WCHAR path[MAX_PATH];
>
> clear_errors();
> @@ -639,15 +715,15 @@ BOOL WINAPI SQLInstallDriverExW(LPCWSTR lpszDriver, LPCWSTR lpszPathIn,
> debugstr_w(lpszPathIn), lpszPathOut, cbPathOutMax, pcbPathOut,
> fRequest, lpdwUsageCount);
>
> - for (p = lpszDriver; *p; p += lstrlenW(p) + 1)
> - TRACE("%s\n", debugstr_w(p));
> + write_registry_values(odbcdrivers, lpszDriver, lpszPathIn, path);
>
> - len = GetSystemDirectoryW(path, MAX_PATH);
> + len = lstrlenW(path);
>
> if (pcbPathOut)
> *pcbPathOut = len;
>
> - len = GetSystemDirectoryW(path, MAX_PATH);
> + if(lpdwUsageCount)
> + *lpdwUsageCount = 1;
This is probably fine for a first version, but usage count handling is also very important
and will require various changes to the registry code (for example, checking for other
matching drivers and incrementing usage count) - so I would suggest to look at this early
enough to avoid huge rewrites.
>
> if (lpszPathOut && cbPathOutMax > len)
> {
> @@ -661,7 +737,6 @@ BOOL WINAPI SQLInstallDriverEx(LPCSTR lpszDriver, LPCSTR lpszPathIn,
> LPSTR lpszPathOut, WORD cbPathOutMax, WORD *pcbPathOut,
> WORD fRequest, LPDWORD lpdwUsageCount)
> {
> - LPCSTR p;
> LPWSTR driver, pathin;
> WCHAR pathout[MAX_PATH];
> BOOL ret;
> @@ -672,9 +747,6 @@ BOOL WINAPI SQLInstallDriverEx(LPCSTR lpszDriver, LPCSTR lpszPathIn,
> debugstr_a(lpszPathIn), lpszPathOut, cbPathOutMax, pcbPathOut,
> fRequest, lpdwUsageCount);
>
> - for (p = lpszDriver; *p; p += lstrlenA(p) + 1)
> - TRACE("%s\n", debugstr_a(p));
> -
> driver = SQLInstall_strdup_multi(lpszDriver);
> pathin = SQLInstall_strdup(lpszPathIn);
>
> diff --git a/dlls/odbccp32/tests/misc.c b/dlls/odbccp32/tests/misc.c
> index 505ce1b..3ae1fd3 100644
> --- a/dlls/odbccp32/tests/misc.c
> +++ b/dlls/odbccp32/tests/misc.c
> @@ -412,6 +412,64 @@ static void test_SQLGetPrivateProfileStringW(void)
> }
> }
>
> +void test_SQLInstallDriverEx(void)
> +{
> + char path[MAX_PATH];
> + char syspath[MAX_PATH];
> + WORD size = 0;
> + BOOL ret, sql_ret;
> + DWORD cnt, error_code = 0;
> + HKEY hkey;
> + DWORD reg_ret;
> +
> + GetSystemDirectoryA(syspath, MAX_PATH);
> +
> + ret = SQLInstallDriverEx("WINE ODBC Driver\0Driver=sample.dll\0Setup=sample.dll\0\0", NULL, path, MAX_PATH, &size, ODBC_INSTALL_COMPLETE, NULL);
Wouldn't it make sense to check ret? Otherwise you can remove this unnecessary assignment.
Also, one terminating null "\0" at the end should be sufficient because the second one is added by the compiler.
> + sql_ret = SQLInstallerErrorW(1, &error_code, NULL, 0, NULL);
> + if (sql_ret && error_code == ODBC_ERROR_WRITING_SYSINFO_FAILED)
> + {
> + win_skip("not enough privileges\n");
> + return;
> + }
> + ok(sql_ret && error_code == SQL_SUCCESS, "SQLInstallDriverEx failed %d, %u\n", sql_ret, error_code);
> + ok(!strcmp(path, syspath), "invalid path %s\n", path);
> +
> + ret = SQLInstallDriverEx("WINE ODBC Driver Path\0Driver=sample.dll\0Setup=sample.dll\0\0", "c:\\temp", path, MAX_PATH, &size, ODBC_INSTALL_COMPLETE, NULL);
> + sql_ret = SQLInstallerErrorW(1, &error_code, NULL, 0, NULL);
> + ok(sql_ret && error_code == SQL_SUCCESS, "SQLInstallDriverEx failed %d, %u\n", sql_ret, error_code);
> + ok(!strcmp(path, "c:\\temp"), "invalid path %s\n", path);
> +
> + if(ret)
> + {
If this code should always execute fine, you can remove the check for "ret". Otherwise, if it
doesn't run on some systems, please add a win_skip(...) message.
> + reg_ret = RegOpenKeyExA(HKEY_LOCAL_MACHINE, "Software\\ODBC\\ODBCINST.INI\\WINE ODBC Driver", 0, KEY_READ, &hkey);
> + ok(reg_ret == ERROR_SUCCESS, "RegOpenKeyExW failed\n");
> + if (reg_ret == ERROR_SUCCESS)
> + {
> + DWORD type, size = MAX_PATH;
> + char driverpath[MAX_PATH];
> +
> + strcpy(driverpath, syspath);
> + strcat(driverpath, "\\sample.dll");
> +
> + reg_ret = RegGetValueA(hkey, NULL, "Driver", RRF_RT_REG_SZ, &type, &path, &size);
> + ok(reg_ret == ERROR_SUCCESS, "RegGetValueA failed\n");
> + ok(!strcmp(path, driverpath), "invalid path %s\n", path);
> +
> + RegCloseKey(hkey);
> + }
> + }
> +
> + cnt = 100;
> + ret = SQLRemoveDriver("WINE ODBC Driver", FALSE, &cnt);
> + ok(ret, "SQLRemoveDriver failed\n");
> + todo_wine ok(cnt == 0, "SQLRemoveDriver failed %d\n", cnt);
> +
> + cnt = 100;
> + ret = SQLRemoveDriver("WINE ODBC Driver Path", FALSE, &cnt);
> + ok(ret, "SQLRemoveDriver failed\n");
> + todo_wine ok(cnt == 0, "SQLRemoveDriver failed %d\n", cnt);
> +}
> +
> START_TEST(misc)
> {
> test_SQLConfigMode();
> @@ -420,4 +478,5 @@ START_TEST(misc)
> test_SQLWritePrivateProfileString();
> test_SQLGetPrivateProfileString();
> test_SQLGetPrivateProfileStringW();
> + test_SQLInstallDriverEx();
> }
>
More information about the wine-devel
mailing list