Re: [PATCH 01/16] ncrypt: Implement NCryptEnumStorageProviders

[Hans Leidekker]

Hi Ariel,

Thanks for your contribution.
> diff --git a/dlls/ncrypt/main.c b/dlls/ncrypt/main.c
> index c0cc05c43ca..86bc9190717 100644
> --- a/dlls/ncrypt/main.c
> +++ b/dlls/ncrypt/main.c
> @@ -23,7 +23,9 @@
>  #include "windef.h"
>  #include "winbase.h"
>  #include "ncrypt.h"
> +#include "winreg.h"
>  #include "wine/debug.h"
> +#include "wine/heap.h"
>
> WINE_DEFAULT_DEBUG_CHANNEL(ncrypt);
>
> @@ -89,6 +91,123 @@ SECURITY_STATUS WINAPI NCryptEnumKeys(NCRYPT_PROV_HANDLE provider, const WCHAR *
>      return NTE_NOT_SUPPORTED;
>  }
>
> +SECURITY_STATUS WINAPI NCryptEnumStorageProviders(DWORD *providerCount, NCryptProviderName **providerList, DWORD flags)
> +{
> - HKEY hKey;
> - HKEY hSubKey;
> - SECURITY_STATUS ret;
> - LSTATUS status;
> - DWORD maxKeyLen;
> - DWORD i;
> - WCHAR providerName[MAX_PATH];
> - WCHAR providerKSReg[MAX_PATH];
> - DWORD providerNameLength;
> - NCryptProviderName *outList;
> - DWORD outCount;
> - DWORD numKeys;
> - void *allocBuffer;
> - void *reAllocBuffer;
> - DWORD allocSize;
> - DWORD totalNamesSize;

We try to avoid Hungarian notation. Please use names like out_count, num_keys, etc.
> - TRACE("(%p, %p, 0x%08x)\n", providerCount, providerList, flags);
> -
> - if (flags)
> - {
> -
>    FIXME("Ignoring flags: 0x%08x - Not implemented\n", flags);
> - }
> -
> - ret = ERROR_SUCCESS;
> - outList = NULL;
> - outCount = 0;
> - allocSize = 0;
> -
> - status = RegOpenKeyExW(HKEY_LOCAL_MACHINE, L"SYSTEM\CurrentControlSet\Control\Cryptography\Providers", 0, KEY_READ, &hKey);
> - if (status != ERROR_SUCCESS)
> - {
> -
>    ERR("Failed to open 'HKLM\\SYSTEM\\CurrentControlSet\\Control\\Cryptography\\Providers' error code: 0x%08x\n", status);

This should be a WARN or TRACE at most.
> -
>    return NTE_NOT_SUPPORTED; /* FIXME: what should be returned here? */
> - }
> -
> - status = RegQueryInfoKeyW(hKey, NULL, NULL, NULL, &numKeys, &maxKeyLen, NULL, NULL, NULL, NULL, NULL, NULL);
> - if (status != ERROR_SUCCESS)
> - {
> -
>    ERR("Failed to query info on 'HKLM\\SYSTEM\\CurrentControlSet\\Control\\Cryptography\\Providers' error code: 0x%08x\n", status);
> -
>    ret = NTE_NOT_SUPPORTED; /* FIXME: what should be returned here? */
> -
>    goto cleanup;
> - }
> -
> -
> - for (i = 0; i < numKeys; ++i)
> - {
> -
>    providerNameLength = MAX_PATH;
> -
>    status = RegEnumKeyExW(hKey, i, providerName, &providerNameLength, NULL, NULL, NULL, NULL);
> -
>    if (status != ERROR_SUCCESS)
> -
>    {
> -
>        ERR("Failed to enumerate sub keys of 'HKLM\\SYSTEM\\CurrentControlSet\\Control\\Cryptography\\Providers' error code: 0x%08x\n", status);
> -
>        ret = NTE_NOT_SUPPORTED; /* FIXME: what should be returned here? */
> -
>        goto cleanup;
> -
>    }
> -
> -
> -
>    /* This registry key should exist if the provider supports key storage */
> -
>    swprintf(providerKSReg, MAX_PATH - 1, L"%s\\UM\\00010001", providerName);
> -
>    status = RegOpenKeyExW(hKey, providerKSReg, 0, KEY_READ, &hSubKey);
> -
>    if (status != ERROR_SUCCESS)
> -
>    {
> -
>        /* Provider doesn't support key storage, skip it */
> -
>        continue;
> -
>    }
> -
>    RegCloseKey(hSubKey);
> -
> -
>    allocSize += sizeof(NCryptProviderName) + (providerNameLength + 1) * sizeof(WCHAR);
> -
>    if (!outCount++)
> -
>    {
> -
>        allocBuffer = heap_alloc(allocSize);
> -
>        if (!allocBuffer)
> -
>        {
> -
>            ERR("Out of memory!\n");

I don't think this is worth an error message.
> -
>            ret = NTE_NO_MEMORY;
> -
>            goto cleanup;
> -
>        }
> -
>        totalNamesSize = 0;
> -
>    }
> -
>    else
> -
>    {
> -
>        reAllocBuffer = heap_realloc(allocBuffer, allocSize);
> -
>        if (!reAllocBuffer)
> -
>        {
> -
>            ERR("Out of memory during realloc!\n");
> -
>            heap_free(allocBuffer);
> -
>            ret = NTE_NO_MEMORY;
> -
>            goto cleanup;
> -
>        }
> -
>        /* Shift the wstrings that contain the names over by the size of one struct */
> -
>        memmove((BYTE*)allocBuffer + outCount * sizeof(NCryptProviderName), (BYTE*)allocBuffer + (outCount - 1) * sizeof(NCryptProviderName), totalNamesSize);
> -
>    }

The reallocated buffer may have moved. You don't need these casts if
you make allocBuffer a BYTE *.
> -
> -
> -
> -
>    outList = allocBuffer;
> -
>    outList[outCount - 1].pszName = (WCHAR*)((BYTE*)allocBuffer + outCount * sizeof(NCryptProviderName) + totalNamesSize);
> -
>    outList[outCount - 1].pszComment = NULL;
> -
> -
>    wcscpy(outList[outCount - 1].pszName, providerName);
> -
>    totalNamesSize += (providerNameLength + 1) * sizeof(WCHAR);
> - }
> -
> -
>
> +cleanup:
> - if (ret == ERROR_SUCCESS)
> - {
> -
>    *providerCount = outCount;
> -
>    *providerList = outList;
> - }
> -
> - RegCloseKey(hKey);
> - return ret;
>
> +}
> -
>
> SECURITY_STATUS WINAPI NCryptFinalizeKey(NCRYPT_KEY_HANDLE key, DWORD flags)
>  {
>      FIXME("(0x%lx, 0x%08x): stub\n", key, flags);
> @@ -97,8 +216,12 @@ SECURITY_STATUS WINAPI NCryptFinalizeKey(NCRYPT_KEY_HANDLE key, DWORD flags)
>
> SECURITY_STATUS WINAPI NCryptFreeBuffer(PVOID buf)
>  {
> - FIXME("(%p): stub\n", buf);
> - return NTE_NOT_SUPPORTED;
> - /* FIXME: How do we tell which provider should be called?
> -
>            How do we update refCounts of referenced objects? */
> - FIXME("(%p): semi-stub\n", buf);
> -
> - heap_free(buf);
> - return ERROR_SUCCESS;
>
> }
>
> SECURITY_STATUS WINAPI NCryptFreeObject(NCRYPT_HANDLE object)

Some tests would be nice.