gdiplus: Add test cases for GdipDrawCurve [corrected]

Andrew Eikum andrew at brightnightgames.com
Sat Jun 6 10:42:57 CDT 2009


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



More information about the wine-devel mailing list