Re: gdiplus: Add test cases for GdipDrawCurve [corrected]
This patch was submitted back on Tuesday and I haven't received a response one way or the other about it. Does anyone see anything immediately wrong with it? I more-or-less copied the functionality of test_GdipDrawBezier right above, testing each of the different input possibilities for correctness. It passes 100% on WinXP SP3 and Win7 RC1, although there are failures in Wine's GdipDrawCurve implementation. Thanks for taking a look, Andrew
Andrew Eikum wrote:
This patch was submitted back on Tuesday and I haven't received a response one way or the other about it. Does anyone see anything immediately wrong with it?
I more-or-less copied the functionality of test_GdipDrawBezier right above, testing each of the different input possibilities for correctness. It passes 100% on WinXP SP3 and Win7 RC1, although there are failures in Wine's GdipDrawCurve implementation.
Thanks for taking a look, Andrew
Hi Andrew, Test crashes on my box: graphics.c:108: Test marked todo: Expected 00000000, got 00000007 wine: Unhandled page fault on write access to 0x00000000 at address 0x6039a113 (thread 0009), starting debugger... Unhandled exception: page fault on write access to 0x00000000 in 32-bit code (0x6039a113). Register dump: CS:0023 SS:002b DS:002b ES:002b FS:0063 GS:006b EIP:6039a113 ESP:0032fc50 EBP:0032fcc8 EFLAGS:00010202( R- -- I - - - ) EAX:00000000 EBX:603be1b4 ECX:41a00000 EDX:00000000 ESI:42200000 EDI:00000000 Stack dump: 0x0032fc50: 00000000 00000000 42200000 41a00000 0x0032fc60: 3e99999a 0032fc9c 0032fc94 60366806 0x0032fc70: 00000578 00000002 7b82c261 7b881f95 0x0032fc80: 00000000 00000002 0032fcc8 00124548 0x0032fc90: 00000578 40c00000 6033ab2d 41400000 0x0032fca0: 6036ed54 fffffffb 6033ab39 00000000 Backtrace: =>0 0x6039a113 GdipDrawCurve2+0x12c() in gdiplus (0x0032fcc8) 1 0x60399e8d GdipDrawCurve+0xa0() in gdiplus (0x0032fd08) 2 0x60343ca0 test_GdipDrawCurve+0x31b() in gdiplus_test (0x0032fd58) 3 0x60349969 func_graphics+0x69() in gdiplus_test (0x0032fd98) 4 0x603670d3 run_test+0x9f() in gdiplus_test (0x0032fdf8) 5 0x60367409 main+0x204() in gdiplus_test (0x0032fea8) 6 0x6036748e __wine_spec_exe_entry+0x6a() in gdiplus_test (0x0032fee8) 7 0x7b87dada start_process+0x158() in kernel32 (0x0032ffe8) 8 0x6002b997 wine_switch_to_stack+0x17() in libwine.so.1 (0x00000000) 0x6039a113 GdipDrawCurve2+0x12c in gdiplus: movl %edx,0x0(%eax) Not sure if that was the reason for not being committed though as the tests could run fine on AJ's magic box of course. -- Cheers, Paul.
Paul Vriens wrote:
Andrew Eikum wrote:
This patch was submitted back on Tuesday and I haven't received a response one way or the other about it. Does anyone see anything immediately wrong with it?
I more-or-less copied the functionality of test_GdipDrawBezier right above, testing each of the different input possibilities for correctness. It passes 100% on WinXP SP3 and Win7 RC1, although there are failures in Wine's GdipDrawCurve implementation.
Thanks for taking a look, Andrew
Hi Andrew,
Test crashes on my box:
Not sure if that was the reason for not being committed though as the tests could run fine on AJ's magic box of course.
+ /* InvalidParameter cases: invalid count */ + status = GdipDrawCurve(graphics, pen, points, -1); + expect(InvalidParameter, status); + + status = GdipDrawCurve(graphics, pen, points, 0); + expect(InvalidParameter, status); This could be a problem. Count isn't checked on allocation in GdipDrawCurve2(), and allocated buffer isn't checked for NULL too.
Nikolay Sivov wrote:
Paul Vriens wrote:
Andrew Eikum wrote:
This patch was submitted back on Tuesday and I haven't received a response one way or the other about it. Does anyone see anything immediately wrong with it?
I more-or-less copied the functionality of test_GdipDrawBezier right above, testing each of the different input possibilities for correctness. It passes 100% on WinXP SP3 and Win7 RC1, although there are failures in Wine's GdipDrawCurve implementation.
Thanks for taking a look, Andrew
Hi Andrew,
Test crashes on my box:
Not sure if that was the reason for not being committed though as the tests could run fine on AJ's magic box of course.
+ /* InvalidParameter cases: invalid count */ + status = GdipDrawCurve(graphics, pen, points, -1); + expect(InvalidParameter, status); + + status = GdipDrawCurve(graphics, pen, points, 0); + expect(InvalidParameter, status);
This could be a problem. Count isn't checked on allocation in GdipDrawCurve2(), and allocated buffer isn't checked for NULL too. Well, even if the tests cause it to crash, doesn't that make this a problem with GdipDrawCurve2 and not the tests patch? The tests do not crash on Windows, which means there's nothing wrong with the tests themselves. If tests are not ever supposed to crash Wine like this, wouldn't it be more appropriate to make a test harness that catches all exceptions and reports as failures? I'm just not seeing how this crash is the _tests'_ fault.
In any case, the problem is as you described. The return of GdipAlloc isn't checked, causing a crash when it tries to access the structures it expects to be there. Would these tests be accepted if there was an accompanying patch to fix this defect, eliminating the crash?
Andrew Eikum wrote:
Nikolay Sivov wrote:
Paul Vriens wrote:
Andrew Eikum wrote:
This patch was submitted back on Tuesday and I haven't received a response one way or the other about it. Does anyone see anything immediately wrong with it?
I more-or-less copied the functionality of test_GdipDrawBezier right above, testing each of the different input possibilities for correctness. It passes 100% on WinXP SP3 and Win7 RC1, although there are failures in Wine's GdipDrawCurve implementation.
Thanks for taking a look, Andrew
Hi Andrew,
Test crashes on my box:
Not sure if that was the reason for not being committed though as the tests could run fine on AJ's magic box of course.
+ /* InvalidParameter cases: invalid count */ + status = GdipDrawCurve(graphics, pen, points, -1); + expect(InvalidParameter, status); + + status = GdipDrawCurve(graphics, pen, points, 0); + expect(InvalidParameter, status);
This could be a problem. Count isn't checked on allocation in GdipDrawCurve2(), and allocated buffer isn't checked for NULL too. Well, even if the tests cause it to crash, doesn't that make this a problem with GdipDrawCurve2 and not the tests patch? The tests do not crash on Windows, which means there's nothing wrong with the tests themselves. If tests are not ever supposed to crash Wine like this, wouldn't it be more appropriate to make a test harness that catches all exceptions and reports as failures? I'm just not seeing how this crash is the _tests'_ fault. Sure, it isn't a test problem, it's a patch problem. Any patch (or series) shouldn't break tests.
In any case, the problem is as you described. The return of GdipAlloc isn't checked, causing a crash when it tries to access the structures it expects to be there. Would these tests be accepted if there was an accompanying patch to fix this defect, eliminating the crash? Exactly, you should patch code too, not only tests. In this particular case it's better to check count first and return InvalidParameter and check for GdipAlloc result and return OutOfMemory if it's NULL.
Nikolay Sivov wrote:
Andrew Eikum wrote:
Nikolay Sivov wrote:
Paul Vriens wrote:
Andrew Eikum wrote:
This patch was submitted back on Tuesday and I haven't received a response one way or the other about it. Does anyone see anything immediately wrong with it?
I more-or-less copied the functionality of test_GdipDrawBezier right above, testing each of the different input possibilities for correctness. It passes 100% on WinXP SP3 and Win7 RC1, although there are failures in Wine's GdipDrawCurve implementation.
Thanks for taking a look, Andrew
Hi Andrew,
Test crashes on my box:
Not sure if that was the reason for not being committed though as the tests could run fine on AJ's magic box of course.
+ /* InvalidParameter cases: invalid count */ + status = GdipDrawCurve(graphics, pen, points, -1); + expect(InvalidParameter, status); + + status = GdipDrawCurve(graphics, pen, points, 0); + expect(InvalidParameter, status);
This could be a problem. Count isn't checked on allocation in GdipDrawCurve2(), and allocated buffer isn't checked for NULL too. Well, even if the tests cause it to crash, doesn't that make this a problem with GdipDrawCurve2 and not the tests patch? The tests do not crash on Windows, which means there's nothing wrong with the tests themselves. If tests are not ever supposed to crash Wine like this, wouldn't it be more appropriate to make a test harness that catches all exceptions and reports as failures? I'm just not seeing how this crash is the _tests'_ fault. Sure, it isn't a test problem, it's a patch problem. Any patch (or series) shouldn't break tests.
In any case, the problem is as you described. The return of GdipAlloc isn't checked, causing a crash when it tries to access the structures it expects to be there. Would these tests be accepted if there was an accompanying patch to fix this defect, eliminating the crash? Exactly, you should patch code too, not only tests. In this particular case it's better to check count first and return InvalidParameter and check for GdipAlloc result and return OutOfMemory if it's NULL. Okay, excellent. I'll make the patch for GdipDrawCurve2 and send that off. Would it be a good idea to re-send the tests patch, or just refer to it in the patch email?
Thank you! Andrew
Andrew Eikum wrote:
Nikolay Sivov wrote:
Exactly, you should patch code too, not only tests. In this particular case it's better to check count first and return InvalidParameter and check for GdipAlloc result and return OutOfMemory if it's NULL. Okay, excellent. I'll make the patch for GdipDrawCurve2 and send that off. Would it be a good idea to re-send the tests patch, or just refer to it in the patch email? Just make it a single patch with GdipDrawCurve2 changes and tests in it. Thank you! Andrew
participants (3)
-
Andrew Eikum -
Nikolay Sivov -
Paul Vriens