Re: imagehlp: improve last error handling in ImageGetCertificateData

[Juan Lang]

Hi Aric,

-    if( !Certificate )
+    if( *RequiredLength < size )
     {
         *RequiredLength = size;
-        return TRUE;
+        SetLastError( ERROR_INSUFFICIENT_BUFFER );
+        return FALSE;
     }

-    if( *RequiredLength < size )
+    if( !Certificate )
     {
         *RequiredLength = size;
-        SetLastError( ERROR_INSUFFICIENT_BUFFER );
-        return FALSE;
+        return TRUE;
     }

Are you sure this is the order that *RequiredLength and Certificate
are checked in?  Seems backwards to the normal style to me.  I won't
bug you for a proper regression test, as I failed to produce one too
:)
--Juan

imagehlp: improve last error handling in ImageGetCertificateData

[Aric Stewart]

Humm,  I wrote a test program mimicking the calls in the application I 
am working on and found this behavior. The app was passing in a NULL 
buffer and a pointer for the size set to 0,  We where exiting with TRUE 
and not setting the last error correctly which was confusing the app. 
Testing on windows seems to show that we needed to set the size and set 
last error (and return FALSE).

I am not sure how i would set up a regression test since wouldn't we 
need an exe with a certificate in it to test with? There must be a 
programmatic way to set that up.

-aric

Juan Lang wrote:
> Hi Aric,
>
> -    if( !Certificate )
> +    if( *RequiredLength < size )
>      {
>          *RequiredLength = size;
> -        return TRUE;
> +        SetLastError( ERROR_INSUFFICIENT_BUFFER );
> +        return FALSE;
>      }
>
> -    if( *RequiredLength < size )
> +    if( !Certificate )
>      {
>          *RequiredLength = size;
> -        SetLastError( ERROR_INSUFFICIENT_BUFFER );
> -        return FALSE;
> +        return TRUE;
>      }
>
> Are you sure this is the order that *RequiredLength and Certificate
> are checked in?  Seems backwards to the normal style to me.  I won't
> bug you for a proper regression test, as I failed to produce one too
> :)
> --Juan

imagehlp: improve last error handling in ImageGetCertificateData

[Juan Lang]

> Humm,  I wrote a test program mimicking the calls in the application I
> am working on and found this behavior. The app was passing in a NULL
> buffer and a pointer for the size set to 0,  We where exiting with TRUE
> and not setting the last error correctly which was confusing the app.

In that case, this part of the patch is still suspect:

-    if( *RequiredLength < size )
+    if( !Certificate )
     {
         *RequiredLength = size;
-        SetLastError( ERROR_INSUFFICIENT_BUFFER );
-        return FALSE;
+        return TRUE;

I'm not sure how Windows behaves in this case, but if it returns FALSE
(ERROR_INSUFFICIENT_BUFFER) when Certificate is NULL and
*RequiredLength is 0, I'd be surprised if it returned TRUE when
Certificate is NULL and *RequiredLength is large enough.
> I am not sure how i would set up a regression test since wouldn't we
> need an exe with a certificate in it to test with? There must be a
> programmatic way to set that up.

Indeed, I thought the same thing, and tried copying in bits from
kernel32/tests/process.c to create an image.  I failed to create one
that Windows would accept, so I gave up.  You can blame that on my
laziness, though.  Feel free to try yourself.
--Juan

imagehlp: improve last error handling in ImageGetCertificateData

[Aric Stewart]

Indeed testing with my test app i see that if the Len is sufficient but 
the buffer is null i get a return of FALSE and a last error of 
ERROR_INVALID_PARAMETER.  I will update my patch.

-aric

Juan Lang wrote:
> > Humm,  I wrote a test program mimicking the calls in the application I
> > am working on and found this behavior. The app was passing in a NULL
> > buffer and a pointer for the size set to 0,  We where exiting with TRUE
> > and not setting the last error correctly which was confusing the app.
>
> In that case, this part of the patch is still suspect:
>
> -    if( *RequiredLength < size )
> +    if( !Certificate )
>      {
>          *RequiredLength = size;
> -        SetLastError( ERROR_INSUFFICIENT_BUFFER );
> -        return FALSE;
> +        return TRUE;
>
> I'm not sure how Windows behaves in this case, but if it returns FALSE
> (ERROR_INSUFFICIENT_BUFFER) when Certificate is NULL and
> *RequiredLength is 0, I'd be surprised if it returned TRUE when
> Certificate is NULL and *RequiredLength is large enough.
> > I am not sure how i would set up a regression test since wouldn't we
> > need an exe with a certificate in it to test with? There must be a
> > programmatic way to set that up.
>
> Indeed, I thought the same thing, and tried copying in bits from
> kernel32/tests/process.c to create an image.  I failed to create one
> that Windows would accept, so I gave up.  You can blame that on my
> laziness, though.  Feel free to try yourself.
> --Juan