Re: [PATCH] wintrust: Implement CryptCATAdminCalcHashFromFileHandle

[Juan Lang]

Hi Maarten,

overall this patch looks pretty good.  A few minor comments:

+        tempbuffer = HeapAlloc(GetProcessHeap(), 0, TEMP_BLOCK_SIZE);
+        if (!tempbuffer)
+            goto out;

You return TRUE in this case.  You probably ought to set last error
and return FALSE instead.

+        while (ReadFile(hFile, tempbuffer, TEMP_BLOCK_SIZE,
&readbytes, NULL) && readbytes)
+            CryptHashData(hash, tempbuffer, readbytes, 0);

Why not while ((ret = ReadFile...) && readbytes) instead?  This way
you can at least return failure if reading the file fails for whatever
reason.

+    SetLastError(ERROR_INSUFFICIENT_BUFFER);
     return TRUE;

Why are you setting ERROR_INSUFFICIENT_BUFFER in the success case?  If
it's just to pass that test, I think you might remove the test
instead:  in general we don't like to check last error in case of
success, unless we know an app does so.
--Juan

[PATCH] wintrust: Implement CryptCATAdminCalcHashFromFileHandle

[Juan Lang]

> - SetLastError(ERROR_INSUFFICIENT_BUFFER);
> return TRUE;
>
> Why are you setting ERROR_INSUFFICIENT_BUFFER in the success case?

Oops, just read the patch more closely.  Please ignore this comment, I
obviously missed the return statement above it.
--Juan

[PATCH] wintrust: Implement CryptCATAdminCalcHashFromFileHandle

[Maarten Lankhorst]

Juan Lang schreef:
> Hi Maarten,
>
> overall this patch looks pretty good.  A few minor comments:
> -
>    tempbuffer = HeapAlloc(GetProcessHeap(), 0, TEMP_BLOCK_SIZE);
> -
>    if (!tempbuffer)
> -
>        goto out;
>
> You return TRUE in this case.  You probably ought to set last error
> and return FALSE instead.

Oops, thanks for catching that. I think HeapAlloc will already set the 
last error correctly, so I don't have to worry about that.
> -
>    while (ReadFile(hFile, tempbuffer, TEMP_BLOCK_SIZE,
>
> &readbytes, NULL) && readbytes)
> -
>        CryptHashData(hash, tempbuffer, readbytes, 0);
>
> Why not while ((ret = ReadFile...) && readbytes) instead?  This way
> you can at least return failure if reading the file fails for whatever
> reason.

I'll fix that too, thanks.
> - SetLastError(ERROR_INSUFFICIENT_BUFFER);
> return TRUE;
>
> Why are you setting ERROR_INSUFFICIENT_BUFFER in the success case?  If
> it's just to pass that test, I think you might remove the test
> instead:  in general we don't like to check last error in case of
> success, unless we know an app does so.

It is in the success case where no buffer is allocated (eg it wants to 
know the buffer size).
It might be better to just wrap it in an 'else { ... }' to be more 
specific about this.

Cheers,
Maarten.

[PATCH] wintrust: Implement CryptCATAdminCalcHashFromFileHandle

[Juan Lang]

> I think HeapAlloc will already set the last
> error correctly, so I don't have to worry about that.

It doesn't.
> It might be better to just wrap it in an 'else { ... }' to be more specific
> about this.

Yeah, an else block would be clearer to me, but I didn't want to
dictate style ;-)

Thanks,
--Juan