Re: [PATCH 3/3] shell32/autocomplete: Revamp pwzsRegKeyPath handling so it can deal with arbitrary sizes and make it more robust

[Alexandre Julliard]

Gabriel Ivăncescu  writes:
> - if (pwzsRegKeyPath)
> - {
> -
>    WCHAR *key, *value;
> -
>    HKEY hKey;
> -
>    LSTATUS res;
> -
>    size_t len;
> -
> -
>    /* pwszRegKeyPath contains the key as well as the value, so split it */
> -
>    value = strrchrW(pwzsRegKeyPath, '\\');
> -
>    len = value - pwzsRegKeyPath;
> -
> -
>    if (value && (key = heap_alloc((len+1) * sizeof(*key))) != NULL) {
> -
>        memcpy(key, pwzsRegKeyPath, len * sizeof(*key));
> -
>        key[len] = '\0';
> -
>        value++;
> -
> -
>        res = RegOpenKeyExW(HKEY_CURRENT_USER, key, 0, KEY_READ, &hKey);
> -
> -
>        /* if not found, MSDN states we must seek in HKEY_LOCAL_MACHINE */
> -
>        if (res != ERROR_SUCCESS)
> -
>            res = RegOpenKeyExW(HKEY_LOCAL_MACHINE, key, 0, KEY_READ, &hKey);
> -
> -
>        if (res == ERROR_SUCCESS) {
> -
>            WCHAR *qc;
> -
>            DWORD type, sz;
> -
>            res = RegQueryValueExW(hKey, value, NULL, &type, NULL, &sz);
> -
> -
>            if (res != ERROR_SUCCESS || type != REG_SZ) {
> -
>                RegCloseKey(hKey);
> -
>                res = RegOpenKeyExW(HKEY_LOCAL_MACHINE, key, 0, KEY_READ, &hKey);
> -
>                if (res != ERROR_SUCCESS)
> -
>                    goto free_key_str;
> -
>                res = RegQueryValueExW(hKey, value, NULL, &type, NULL, &sz);
> -
>            }
> -
> -
>            /* the size does include the NUL terminator, however, strings
> -
>               can be stored without it, so make room for NUL just in case */
> -
>            if (res == ERROR_SUCCESS && type == REG_SZ &&
> -
>                (qc = heap_alloc(sz + sizeof(*qc))) != NULL)
> -
>            {
> -
>                DWORD old_sz = sz;
> -
>                res = RegQueryValueExW(hKey, value, NULL, &type, (BYTE*)qc, &sz);
> -
> -
>                /* make sure the value wasn't messed with */
> -
>                if (res == ERROR_SUCCESS && sz == old_sz && type == REG_SZ) {
> -
>                    qc[sz / sizeof(*qc)] = '\0';
> -
>                    This->quickComplete = qc;
> -
>                }
> -
>                else
> -
>                    heap_free(qc);
> -
>            }
> -
>            RegCloseKey(hKey);
> -
>        }
> -
>      free_key_str:
> -
>        heap_free(key);
> -
>    }

This doesn't seem like an improvement. In particular, querying the key
twice just to retrieve the length is making things worse, and creating
more race conditions. Also note that RegQueryValueExW already adds a
terminating null if necessary.