[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.