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