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
