Re: comctl32: Don't crash when getting a bad image list handle. (try 2)

[Nikolay Sivov]

On 9/19/2011 17:32, Francois Gouget wrote:
> ---
>
> Don't return from inside the block this time.
>
> dlls/comctl32/imagelist.c       |   13 ++++++++++++-
>   dlls/comctl32/tests/imagelist.c |   17 +++++++++++++++--
>   2 files changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/dlls/comctl32/imagelist.c b/dlls/comctl32/imagelist.c
> index ec62370..83b8b44 100644
> --- a/dlls/comctl32/imagelist.c
> +++ b/dlls/comctl32/imagelist.c
> @@ -54,6 +54,7 @@
>   #include "commoncontrols.h"
>   #include "imagelist.h"
>   #include "wine/debug.h"
> +#include "wine/exception.h"
>
> WINE_DEFAULT_DEBUG_CHANNEL(imagelist);
>
> @@ -3587,7 +3588,17 @@ static const IImageListVtbl ImageListImpl_Vtbl = {
>
> static inline BOOL is_valid(HIMAGELIST himl)
>   {
> -    return himl&&  himl->lpVtbl ==&ImageListImpl_Vtbl;
> +    BOOL valid;
> +    __TRY
> +    {
> +        valid = himl&&  himl->lpVtbl ==&ImageListImpl_Vtbl;
> +    }
> +    __EXCEPT_PAGE_FAULT
> +    {
> +        valid = FALSE;
> +    }
> +    __ENDTRY
> +    return valid;
>   }
> Maybe use IsBadReadPtr() with sizeof(void*) is better? You need only 
> vtable pointer to be accessible.

comctl32: Don't crash when getting a bad image list handle. (try 2)

[Francois Gouget]

On Mon, 19 Sep 2011, Nikolay Sivov wrote:
[...]
> > static inline BOOL is_valid(HIMAGELIST himl)
> >   {
> > -    return himl&&  himl->lpVtbl ==&ImageListImpl_Vtbl;
> > +    BOOL valid;
> > +    __TRY
> > +    {
> > +        valid = himl&&  himl->lpVtbl ==&ImageListImpl_Vtbl;
> > +    }
> > +    __EXCEPT_PAGE_FAULT
> > +    {
> > +        valid = FALSE;
> > +    }
> > +    __ENDTRY
> > +    return valid;
> >   }
> > Maybe use IsBadReadPtr() with sizeof(void*) is better? You need only vtable
> > pointer to be accessible.

I prefer this construct as using IsBadReadPtr() would introduce a race 
in is_valid(). Of course calling is_valid() is in itself racy anyway. So 
I don't mind switching over if that's preferred (the code would have 
fewer lines).


-- 
Francois Gouget <fgouget(a)free.fr>              http://fgouget.free.fr/
              E-Voting: Those who cast the votes decide nothing.
                 Those who count the votes decide everything.

comctl32: Don't crash when getting a bad image list handle. (try 2)

[Chris Robinson]

On Monday, September 19, 2011 4:09:29 PM Francois Gouget wrote:
> I prefer this construct as using IsBadReadPtr() would introduce a race
> in is_valid(). Of course calling is_valid() is in itself racy anyway. So
> I don't mind switching over if that's preferred (the code would have
> fewer lines).

Would it be feasible to keep a list or array of all pertinent HIMAGELIST 
objects, and just look up the handle in that list for validation? If it's kept 
sorted, you can even do a binary search for it instead of linear.

comctl32: Don't crash when getting a bad image list handle. (try 2)

[Nikolay Sivov]

On 9/19/2011 20:45, Chris Robinson wrote:
> On Monday, September 19, 2011 4:09:29 PM Francois Gouget wrote:
> > I prefer this construct as using IsBadReadPtr() would introduce a race
> > in is_valid(). Of course calling is_valid() is in itself racy anyway. So
> > I don't mind switching over if that's preferred (the code would have
> > fewer lines).
> > Would it be feasible to keep a list or array of all pertinent HIMAGELIST
> > objects, and just look up the handle in that list for validation? If it's kept
> > sorted, you can even do a binary search for it instead of linear.
> > It's not a handle, it's just a pointer.