Opened 14 years ago

Closed 14 years ago

#105 closed defect (fixed)

Division by zero in svghandler.cpp

Reported by: Dmitry A. Kuminov Owned by:
Priority: blocker Milestone: Qt GA
Component: General Version: 4.5.1 Beta 3
Severity: medium Keywords:
Cc:

Description (last modified by Dmitry A. Kuminov)

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 Dmitry A. Kuminov 14 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 14 years ago by Dmitry A. Kuminov

Description: modified (diff)

comment:2 Changed 14 years ago by Dmitry A. Kuminov

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 14 years ago by Dmitry A. Kuminov

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

comment:4 Changed 14 years ago by Silvan Scherrer

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 14 years ago by Silvan Scherrer

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 14 years ago by Dmitry A. Kuminov

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 14 years ago by Dmitry A. Kuminov

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 14 years ago by Dmitry A. Kuminov

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 14 years ago by Dmitry A. Kuminov

Severity: 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 14 years ago by Silvan Scherrer

Milestone: Qt EnhancedQt GA
Severity: lowmedium

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

comment:11 Changed 14 years ago by Silvan Scherrer

Priority: majorblocker

comment:12 Changed 14 years ago by Dmitry A. Kuminov

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 14 years ago by Dmitry A. Kuminov

Attachment: API_FPU_CW_Wrappers.awk added

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

comment:13 Changed 14 years ago by Dmitry A. Kuminov

Resolution: fixed
Status: newclosed

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.