Re: [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  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&ur...
~/.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 :)