Re: crypt32: CryptProtectData/CryptUnprotectData take 3

[Dmitry Timoshkov]

"Kees Cook" <kees(a)outflux.net> wrote:
> diff -u -p -u -p -r1.19 crypt32.spec
> --- dlls/crypt32/crypt32.spec 10 Nov 2004 01:31:50 -0000 1.19
> +++ dlls/crypt32/crypt32.spec 6 Apr 2005 05:52:55 -0000
> @@ -133,6 +133,7 @@
>  @ stub CryptMsgUpdate
>  @ stub CryptMsgVerifyCountersignatureEncoded
>  @ stdcall CryptProtectData(ptr wstr ptr ptr ptr long ptr)
> +@ stdcall CryptUnprotectData(ptr ptr ptr ptr ptr long ptr)
>  @ stdcall CryptRegisterDefaultOIDFunction(long str long wstr)
>  @ stdcall CryptRegisterOIDFunction(long str str wstr str)
>  @ stub CryptRegisterOIDInfo
> @@ -149,7 +150,6 @@
>  @ stub CryptSignHashU
>  @ stub CryptSignMessage
>  @ stub CryptSignMessageWithKey
> -@ stub CryptUnprotectData
>  @ stub CryptUnregisterDefaultOIDFunction
>  @ stub CryptUnregisterOIDFunction
>  @ stub CryptUnregisterOIDInfo

It's better to keep alphabetical order of .spec file entries.
> +#include "config.h"
> +#include <stdarg.h>
> +#include <stdio.h>
> +#include <string.h>
> +
> +#include "windef.h"
> +#include "winbase.h"
> +#include "wincrypt.h"
> +#include "winreg.h"
> +#include "winnls.h"
> +#include "mssip.h"
> +#include "wine/debug.h"
> +
> +WINE_DEFAULT_DEBUG_CHANNEL(crypt);

if you are not going to conditionally include headers using '#ifdef HAVE_xxx'
there is no need to include config.h.
> +        r = RegCreateKeyExW(hkeyMap, wszIndexKey, 0, NULL, REG_OPTION_NON_VOLATILE, KEY_ALL_ACCESS, NULL, &hkeyOpen,
> &dwDisposition);
> +        if (r != ERROR_SUCCESS)
> +           continue;
> +        if (dwDisposition == REG_OPENED_EXISTING_KEY)
> +        {
> +            /* already exists, skip */
> +            CloseHandle(hkeyOpen);

Registry keys should be closed by RegCloseKey, not CloseHandle (here and
everywhere else).
> +    entropy.pbData=(void*)key;
> +    entropy.cbData=strlen(key)+1;
> +
> +    protected = CryptProtectData(NULL,desc,NULL,NULL,NULL,0,&cipher);
> +    ok(!protected, "Encrypting without plain data source.\n");
> +    r = GetLastError();
> +    ok2(r == ERROR_INVALID_PARAMETER, "Wrong (%lu) GetLastError seen\n",r);

If you are going to test last error value after an API call it's a usual practice
to set the error first to some invalid value, 0xdeadbeef works fine.

--
Dmitry.

crypt32: CryptProtectData/CryptUnprotectData take 3

[Kees Cook]

On Wed, Apr 06, 2005 at 11:08:12PM +0900, Dmitry Timoshkov wrote:
> It's better to keep alphabetical order of .spec file entries.

Oh, whoops.  I just blindly grouped them.  Fixed.
> if you are not going to conditionally include headers using '#ifdef HAVE_xxx'
> there is no need to include config.h.

Good point.  This was copied from elsewhere.  I've cleaned up the header 
files.
> Registry keys should be closed by RegCloseKey, not CloseHandle (here and
> everywhere else).

Whoops.  Okay, fixed.
> If you are going to test last error value after an API call it's a usual practice
> to set the error first to some invalid value, 0xdeadbeef works fine.

Added.

Thanks for all the suggestions from everyone!  I'm glad you're willing 
to put up with me.  :)  Take 4 on it's way.

-- 
Kees Cook                                            @outflux.net