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

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.