Re: wintrust: Implement CryptCATAdminCalcHashFromFileHandle, try 2

2008-10-21 Thread Maarten Lankhorst
Hi AJ, Alexandre Julliard schreef: > Maarten Lankhorst <[EMAIL PROTECTED]> writes: > > >> +curpos.QuadPart = 0; >> +SetFilePointerEx(hFile, curpos, &oldpos, FILE_CURRENT); >> +SetFilePointerEx(hFile, curpos, NULL, FILE_BEGIN); >> > > That's not thread-safe. > My

Re: wintrust: Implement CryptCATAdminCalcHashFromFileHandle, try 2

2008-10-21 Thread Alexandre Julliard
Maarten Lankhorst <[EMAIL PROTECTED]> writes: > +curpos.QuadPart = 0; > +SetFilePointerEx(hFile, curpos, &oldpos, FILE_CURRENT); > +SetFilePointerEx(hFile, curpos, NULL, FILE_BEGIN); That's not thread-safe. -- Alexandre Julliard [EMAIL PROTECTED]

Re: wintrust: Implement CryptCATAdminCalcHashFromFileHandle

2008-10-20 Thread Juan Lang
Hi Maarten, +if (pbHash && *pcbHash < 20) +{ +SetLastError(ERROR_INSUFFICIENT_BUFFER); +return FALSE; +} That's not correct, *pcbHash must be set to 20 if it's too small. +while (ReadFile(hFile, tempbuffer, TEMP_BLOCK_SIZE, &readbytes, NULL) && readbytes) +

Re: [PATCH] wintrust: Implement CryptCATAdminCalcHashFromFileHandle

2008-10-16 Thread 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

Re: [PATCH] wintrust: Implement CryptCATAdminCalcHashFromFileHandle

2008-10-16 Thread 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 >

Re: [PATCH] wintrust: Implement CryptCATAdminCalcHashFromFileHandle

2008-10-16 Thread 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

Re: [PATCH] wintrust: Implement CryptCATAdminCalcHashFromFileHandle

2008-10-16 Thread 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. +