Re: [PATCH 2/3] gdiplus: Set outer pointer to NULL when image loading functions fail.

[Dmitry Timoshkov]

Qian Hong <qhong(a)codeweavers.com> wrote:
> @@ -1365,10 +1365,12 @@ GpStatus WINGDIPAPI GdipCreateBitmapFromFile(GDIPCONST WCHAR* filename,
>      if(!filename || !bitmap)
>          return InvalidParameter;
>
> +    *bitmap = NULL;

Is there an application that depends on this behaviour?
> stat = GdipCreateStreamOnFile(filename, GENERIC_READ, &stream);
>
> if(stat != Ok)
> -        return stat;
> +        return InvalidParameter;

Probably GdipCreateStreamOnFile should be fixed instead (with tests
of course).
> stat = GdipCreateBitmapFromStream(stream, bitmap);
>
> @@ -2923,10 +2925,12 @@ GpStatus WINGDIPAPI GdipLoadImageFromFile(GDIPCONST WCHAR* filename,
>      if (!filename || !image)
>          return InvalidParameter;
>
> +    *image = NULL;

Same question as above.
> stat = GdipCreateStreamOnFile(filename, GENERIC_READ, &stream);
>
> if (stat != Ok)
> -        return stat;
> +        return OutOfMemory;

Same note about GdipCreateStreamOnFile. And I don't think that's a good
idea to replicate weird return error codes without actually getting any
memory allocation errors.

-- 
Dmitry.

[PATCH 2/3] gdiplus: Set outer pointer to NULL when image loading functions fail.

[Qian Hong]

Hi Dmitry,

Thanks for comment :)

On Sun, Mar 2, 2014 at 10:50 AM, Dmitry Timoshkov <dmitry(a)baikal.ru> wrote:
> > +    *bitmap = NULL;
>
> Is there an application that depends on this behaviour?

No, I don't know yet. I hope maybe some app will be benefited from
this change. Any advice?
> > stat = GdipCreateStreamOnFile(filename, GENERIC_READ, &stream);
> >
> > if(stat != Ok)
> > -        return stat;
> > +        return InvalidParameter;
>
> Probably GdipCreateStreamOnFile should be fixed instead (with tests
> of course).

1. I have some tests in [Patch 3/3] which show that the error code
from GdipCreateStreamOnFile doesn't match the error code from
GdipCreateBitmapFromFile.
2. I haven't found any real world app rely on return code from
GdipCreateStreamOnFile, so I leave those todo_wine there.
3. I haven't found any real world app rely on return code from
GdipCreateBitmapFromFile as well, I don't have strong opinion here.

What's your advice regarding the return code here?
> > +    *image = NULL;
>
> Same question as above.

Yes for this one. As the email said, 'Essence Securities.' depends on
this behavior.

http://www.essence.com.cn/essence/softInfoAction.do?method=addSoftLog&urlId=...
~/.wine/drive_c/AX/2003$ WINEDEBUG=+tid,+gdiplus,+seh wine client_rzrq.exe

0009:trace:gdiplus:GdipLoadImageFromFile (L"") 0x329c44
0009:trace:gdiplus:GdipCreateStreamOnFile (L"", 2147483648, 0x329bdc)
...
0009:trace:gdiplus:GdipDrawImageRect (0x1d3ed8, 0x320000, 4.00, -3.00,
30.00, 30.00)
0009:trace:gdiplus:GdipGetImageBounds 0x320000 0x329bc0 0x329bd0
wine: Unhandled page fault on read access to 0x00000000 at address
0x7d33495d (thread 0009), starting debugger...
Backtrace:
=>0 0x7d33495d ipicture_pixel_width+0x15(pic=(nil))
[/home/fracting/wine-git/dlls/gdiplus/../../include/ocidl.h:1221] in
gdiplus (0x00329af8)
  1 0x7d3440fc GdipGetImageBounds+0x141(image=0x320000,
srcRect=0x329bc0, srcUnit=0x329bd0)
[/home/fracting/wine-git/dlls/gdiplus/image.c:2158] in gdiplus
(0x00329b68)
  2 0x7d3239ba GdipDrawImageRect+0xc4(graphics=<couldn't compute
location>, image=<couldn't compute location>, x=<couldn't compute
location>, y=<couldn't compute location>, width=<couldn't compute
location>, height=<couldn't compute location>)
[/home/fracting/wine-git/dlls/gdiplus/graphics.c:3146] in gdiplus
(0x00329bf8)
  3 0x0040f799 in client_rzrq (+0xf798) (0x0032e9a8)
...
0x7d33495d ipicture_pixel_width+0x15
[/home/fracting/wine-git/dlls/gdiplus/../../include/ocidl.h:1221] in
gdiplus: movl 0x0(%eax),%eax
1221    return This->lpVtbl->get_Width(This,pWidth);

1. The program pass &image to GdipLoadImageFromFile, while
image->picture is NULL;
2. GdipLoadImageFromFile of cause didn't load any image because file
name is empty;
3. Unfortunately the program didn't check the return code from
GdipLoadImageFromFile, and pass image to GdipDrawImageRect
4. Finally crashing happen in gdiplus because image->picture is NULL.

 (I have a test for empty file name which has the same result with
non-existent file, so I didn't add it to the patch)
> > stat = GdipCreateStreamOnFile(filename, GENERIC_READ, &stream);
> >
> > if (stat != Ok)
> > -        return stat;
> > +        return OutOfMemory;
>
> Same note about GdipCreateStreamOnFile. And I don't think that's a good
> idea to replicate weird return error codes without actually getting any
> memory allocation errors.

The same situation here:
1. return code from GdipCreateStreamOnFile doesn't match return code
from GdipLoadImageFromFile
2. haven't found real world app reply on the return code yet.

Any advice?

Thanks for every lines of comment :)

-- 
Regards,
Qian Hong

-
http://www.codeweavers.com

[PATCH 2/3] gdiplus: Set outer pointer to NULL when image loading functions fail.

[Dmitry Timoshkov]

Qian Hong <qhong(a)codeweavers.com> wrote:
> > > +    *bitmap = NULL;
> >
> > Is there an application that depends on this behaviour?
>
> No, I don't know yet. I hope maybe some app will be benefited from
> this change. Any advice?

Probably this change is OK since it matches what GdipLoadImageFromFile does.
> > > stat = GdipCreateStreamOnFile(filename, GENERIC_READ, &stream);
> > >
> > > if(stat != Ok)
> > > -        return stat;
> > > +        return InvalidParameter;
> >
> > Probably GdipCreateStreamOnFile should be fixed instead (with tests
> > of course).
>
> 1. I have some tests in [Patch 3/3] which show that the error code
> from GdipCreateStreamOnFile doesn't match the error code from
> GdipCreateBitmapFromFile.
> 2. I haven't found any real world app rely on return code from
> GdipCreateStreamOnFile, so I leave those todo_wine there.
> 3. I haven't found any real world app rely on return code from
> GdipCreateBitmapFromFile as well, I don't have strong opinion here.
>
> What's your advice regarding the return code here?

A common rule is to propagate errors from lower level APIs.
> > > stat = GdipCreateStreamOnFile(filename, GENERIC_READ, &stream);
> > >
> > > if (stat != Ok)
> > > -        return stat;
> > > +        return OutOfMemory;
> >
> > Same note about GdipCreateStreamOnFile. And I don't think that's a good
> > idea to replicate weird return error codes without actually getting any
> > memory allocation errors.
>
> The same situation here:
> 1. return code from GdipCreateStreamOnFile doesn't match return code
> from GdipLoadImageFromFile
> 2. haven't found real world app reply on the return code yet.
>
> Any advice?

There is no need to change something just to replicate behaviour that
you don't really understand.

-- 
Dmitry.

[PATCH 2/3] gdiplus: Set outer pointer to NULL when image loading functions fail.

[Qian Hong]

Hi Dmitry,

Thanks very much for the advice, every comments have been addressed
and a try 2 has been sent. Wish you have a good day :)

[PATCH 2/3] gdiplus: Set outer pointer to NULL when image loading functions fail.

[Dmitry Timoshkov]

Qian Hong <qhong(a)codeweavers.com> wrote:
> Thanks very much for the advice, every comments have been addressed
> and a try 2 has been sent. Wish you have a good day :)

Of course it would be more interesting to test gdiplus image loading
APIs on a real image file instead of a zero-sized one, probably then
you wouldn't get NotImplemented in patch 3/3 either.

-- 
Dmitry.

[PATCH 2/3] gdiplus: Set outer pointer to NULL when image loading functions fail.

[Qian Hong]

Hi Dmitry,

On Wed, Mar 5, 2014 at 3:49 PM, Dmitry Timoshkov <dmitry(a)baikal.ru> wrote:
> > Thanks very much for the advice, every comments have been addressed
> > and a try 2 has been sent. Wish you have a good day :)
>
> Of course it would be more interesting to test gdiplus image loading
> APIs on a real image file instead of a zero-sized one, probably then
> you wouldn't get NotImplemented in patch 3/3 either.

Thanks for your thought, but that not the point of my tests:
+    hfile = CreateFileW(non_shrW, GENERIC_READ, 0, NULL,
OPEN_EXISTING, FILE_FLAG_DELETE_ON_CLOSE, 0);
My point is to test a file without share-reading access, so
GdipCreateStreamOnFile is going to fail no matter if the file is empty
or not.




-- 
Regards,
Qian Hong

-
http://www.codeweavers.com

[PATCH 2/3] gdiplus: Set outer pointer to NULL when image loading functions fail.

[Dmitry Timoshkov]

Qian Hong <qhong(a)codeweavers.com> wrote:
> > Of course it would be more interesting to test gdiplus image loading
> > APIs on a real image file instead of a zero-sized one, probably then
> > you wouldn't get NotImplemented in patch 3/3 either.
>
> Thanks for your thought, but that not the point of my tests:
> +    hfile = CreateFileW(non_shrW, GENERIC_READ, 0, NULL,
> OPEN_EXISTING, FILE_FLAG_DELETE_ON_CLOSE, 0);
> My point is to test a file without share-reading access, so
> GdipCreateStreamOnFile is going to fail no matter if the file is empty
> or not.

But it may not fail or fail some other way in case of a real image.
In any case testing with an empty file is not something that's really
interesting.

-- 
Dmitry.

[PATCH 2/3] gdiplus: Set outer pointer to NULL when image loading functions fail.

[Qian Hong]

On Wed, Mar 5, 2014 at 4:27 PM, Dmitry Timoshkov <dmitry(a)baikal.ru> wrote:
> But it may not fail or fail some other way in case of a real image.
> In any case testing with an empty file is not something that's really
> interesting.

Thanks, I added some tests for real image and broken image instead of
empty file. Guess you will have some comments... ;)


-- 
Regards,
Qian Hong

-
http://www.codeweavers.com