Opened 9 years ago

Closed 9 years ago

#105 closed defect (fixed)

Division by zero in svghandler.cpp

Reported by: dmik Owned by:
Priority: blocker Milestone: Qt GA
Component: General Version: 4.5.1 Beta 3
Severity: medium Keywords:
Cc:

Description (last modified by dmik)

The SVG module has a function, pathArc() defined in source:/trunk/src/svg/qsvghandler.cpp that uses its arguments as divisors w/o checking if they are zero or not.

This function is a reason why e.g. mp3diags traps. First of all, mp3diags somehow supplies a wrong SVG file to QIcon which contains these invalid zero arguments (this needs to be investigated, I suspect it's an OS/2 specific bug). Then, this function causes SIGFPE (which is originally XCPT_FLOAT_INVALID_OPERATION) because of division by zero.

The obvious fix is to replace

    rx = qAbs(rx);
    ry = qAbs(ry);

at around #1285 in svghandler.cpp to

    rx = qAbs(rx);
    ry = qAbs(ry);
    if (rx == 0 || ry == 0)
        return; 

I didn't commit this fix though. This code is in the Qt code base for so long that it would definitely be fixed if it made problems on other platforms (it's very easy to create an invalid .svg to cause zero arguments to be passed in there). But it is still not fixed (see http://qt.gitorious.org/qt/qt/blobs/master/src/svg/qsvghandler.cpp) which means that for some reason it doesn't make any problems anywhere except OS/2.

Attachments (1)

API_FPU_CW_Wrappers.awk (2.6 KB) - added by dmik 9 years ago.
AWK script to create wrappers for Win* and Gpi* calls that restore the FPU Control Word

Download all attachments as: .zip

Change History (14)

comment:1 Changed 9 years ago by dmik

  • Description modified (diff)

comment:2 Changed 9 years ago by dmik

I checked division by zero on Windows and it seems that it doesn't cause SIGFPE -- it returns infinity (which is kind of OK since according to the C standard, see http://en.wikipedia.org/wiki/Division_by_zero#In_computer_arithmetic, it's entirely up to the compiler to decide what to do in this case.

I suspect that it's either kLIBC that instructs the CPU to trigger this exception on OS/2 or it's just the default behavior on OS/2. If someone has any additional info, you are welcome.

comment:3 Changed 9 years ago by dmik

Note that this only applies to floating point arithmetics, in case of integers most (all?) environments will trow an exception.

comment:4 Changed 9 years ago by diver

this is what Knut tells on Irc:
_diver BirdWrk?: could you eventually comment this: http://svn.netlabs.org/qt4/ticket/105

BirdWrk? _diver: somebody (a video driver, PM, GRADD, xxx) screws up the FPU status word, that's a well known issue on OS/2.

BirdWrk? _diver: if you search os2ddprog and other os/2 programming dev lists, you'll find this topic comming up every so often.

BirdWrk? _diver: it is *not* a bug in kLIBC.

comment:5 Changed 9 years ago by diver

Doodle _diver: I also ran into this FPU problem with Cairo,

Doodle _diver: see: http://cgit.freedesktop.org/cairo/tree/src/cairo-os2-surface.c?id=4fc7bdaed651a5c19eb89dddd88808468e0e7eb8
Doodle _diver: (search for "DisableFPUException")

comment:6 Changed 9 years ago by dmik

Thanks for this information. This explains it. Though, here even this simple program

#include <stdio.h>

typedef float real;
#define FMT "%f"

int main()
{
	real a = 1.;
	real b = 0.;
	printf("a=" FMT "b=" FMT "\n", a, b);
	real c = a/b;
	printf("c=" FMT "\n", c);
	return 0;
}

triggers it here (g++ 3.3.5 CSD3).

Maybe it's due to the VirtualBox? GRADD driver (as I run eCS under VirtualBox?).

Silvan, can you run this test on your real hardware?

comment:7 Changed 9 years ago by dmik

Applying a fix similar to DisableFPUException but with the EM_ZERODIVIDE flag (masking off the division by zero) helps indeed. I will try to incorporate this in Qt.

comment:8 Changed 9 years ago by dmik

Okay, placing the FPU CW restore code to the initializer is not enough: Qt apparently calls some Win/Gpi? API that effectively breaks the CW again. Finding this requires some time, so I'm postponing it.

comment:9 Changed 9 years ago by dmik

  • Severity set to low

I think it makes sense to do this for GA as it may cause a lot of potential problems in custom applications that expect that 1.0/0.0 won't cause an exception. Silvan?

The best solution is to simply create wrappers for all Win* calls which will take a day I suppose.

comment:10 Changed 9 years ago by diver

  • Milestone changed from Qt Enhanced to Qt GA
  • Severity changed from low to medium

probably we really need to do it. even i know only one case till now.

comment:11 Changed 9 years ago by diver

  • Priority changed from major to blocker

comment:12 Changed 9 years ago by dmik

Okay, good, the wrapper approach worked. In r430, I committed the header that contains wrappers for all known Win* and Gpi* functions. This header is included from qt_os2.h (which is included in every source that uses OS/2 APIs) so the wrappers are always in action.

I tried to make the wrappers as tiny as possible. They are all inlined (no extra calls and returns), and the Control Word is restored using the assembler directly, so that the resulting overhead in the release build (with the inlines turned on) is just these several 5 asm instructions (~nothing).

I'm also attaching the AWK script that I used to create these wrappers (there are about 400 APIs in total).

With the wrappers, both my tests and MP3Diags with the broken icons work as expected (no SIGFPE). Pol, please check it on your side. Note that it will take quite a while to rebuild Qt after the checkout.

Changed 9 years ago by dmik

AWK script to create wrappers for Win* and Gpi* calls that restore the FPU Control Word

comment:13 Changed 9 years ago by dmik

  • Resolution set to fixed
  • Status changed from new to closed

Elbert reported that mp3diags works now w/o any patch. This task may be considered as done.

Note: See TracTickets for help on using tickets.