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.