[cross-post: povray.bugreports]

Rendering this test code, I can reliably crash POVRay. 

-----------------------------------------------------------
#include "functions.inc"

global_settings { assumed_gamma 2.2 }
light_source { <0,1,0>*20, rgb <2,2,2>/2 }
light_source { <0.2,1,-0.8>*20, rgb <2,2,2>/1.7 }

#declare m=3;
parametric {
        function { u*sin(v) }
        function { 15*sqrt(pow(sin(m*v)*sin(u/2)/3,2)+
                pow(cos(m*v)*m/u*sin(u/2)/3,2)) }
        function { u*cos(v) }
        <0.001,0>, <15,2*pi>
        contained_by {  sphere { 0, 15 }  }
        precompute 12 x,y,z
        accuracy 0.000001
}

camera {  location <0,0.3,-1>*8.3 up y  look_at <0,0,0> }
-----------------------------------------------------------

The system in question is a Linux-ix86 (AthlonXP) box, compiler 
is GCC-3.2. 

The bug may not show up on you box because of it's nature: 

The reason for the bug is uninitialized static data (yeah...). 

The following patch will fix it: 

----------------------------------------------
diff -urN povray-3.50c/src/fpmetric.cpp povray-3.50c-ww/src/fpmetric.cpp
--- povray-3.50c/src/fpmetric.cpp       2003-01-07 02:08:27.000000000 +0100
+++ povray-3.50c-ww/src/fpmetric.cpp    2003-06-20 23:29:54.000000000 +0200
@@ -124,7 +124,8 @@
 static int PrecompLastDepth;
 
 static DBL Intervals_Low[2][32], Intervals_Hi[2][32];
-static int SectorNum[32];
+static int SectorNum[32]={0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
+                          0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0};
 
 

/*****************************************************************************
@@ -357,7 +358,7 @@
                }
 
                /* Z */
-               if ((SectorNum[i] < MaxPrecompZ) && (0 < SectorNum[i]))
+               if (SectorNum[i] < MaxPrecompZ)
                {
                        low = PData->Low[2][SectorNum[i]];
                        hi = PData->Hi[2][SectorNum[i]];
----------------------------------------------

The first chunk simply sets up the static data to sane values. 
It is important that these values are non-negative. 

The second patch should be reviewed before getting applied. 
It removes the check for SectorNum[i] being larger than 0. 

The reason why I am including it here is that this looked to me as 
if somebody did the most trivial patch for the problem I encountered 
but it just cures the symptome, not the bug itself. 

Looking at the code, there is no way how SectorNum[i] could get 
negative. 
Furthermore, the X and Y components do not have such a check, 
only the Z component calculation does. Looks odd to me. 

The only thing one should consider is that the check will prevent 
SectorNum==0 from passing. I see no reason why this would be necessary 
but experts should verify that. 

Wolfgang

P.S. The code I've just read is a masterpiece of "code duplication". 
That's not exactly good and tends to introduce bugs...
Should I post a patch...?

Post a reply to this message

OOPS, I was tricked by the success of my "solution" but the bug 
is actually worse. 

Wolfgang Wieser wrote:
> [cross-post: povray.bugreports]
> 
> Rendering this test code, I can reliably crash POVRay.
> 
> -----------------------------------------------------------
> <snipped>
> -----------------------------------------------------------
> 
Still. Correct

> The bug may not show up on you box because of it's nature:
> 
Correct. 

> The reason for the bug is uninitialized static data (yeah...).
> 
No. Initializing static data is never a bad idea but the actual 
reason for the bug is something else: 

In fpmetric.cpp, around line 430, the following code can be found:

-----------------------------------
                else
                {
                        /* 1 copy */
                        if ((SectorNum[i] *= 2) >= Max_intNumber)
                                SectorNum[i] = Max_intNumber;
                        SectorNum[i + 1] = SectorNum[i];
                        SectorNum[i]++;
                        i++;     // <--- BUG!!
                        Intervals_Low[INDEX_U][i] = low_vect[U];
-------------------------------------

The bug is where I marked it: i is increased but nobody checks 
if it stays in range 0..31 as required by the array sizes of 
Intervals_Low[][] and SectorNum[]. 

So, the code should be changed into something like: 

-----------------------------------
                else
                {
                        /* 1 copy */
+                       if(i>=31)
+                       {  Do something (break, continue, ...)  }
                        if ((SectorNum[i] *= 2) >= Max_intNumber)
                                SectorNum[i] = Max_intNumber;
                        SectorNum[i + 1] = SectorNum[i];
                        SectorNum[i]++;
                        i++;
                        Intervals_Low[INDEX_U][i] = low_vect[U];
-------------------------------------

I am pretty sure this is the actual reason for the bug because 
test output showed the following values for i: 
...
i=33
i=32
i=33
i=32
i=32
SIGSEGV

Wolfgang

BTW, I still consider the check for SectorNum<0 in the Z component 
calculation as unneeded.

Post a reply to this message

In article <3ef38a82@news.povray.org> , Wolfgang Wieser <wwi### [at] gmxde>  
wrote:

> The bug is where I marked it: i is increased but nobody checks
> if it stays in range 0..31 as required by the array sizes of
> Intervals_Low[][] and SectorNum[].

Yes, the isosurface method 1 was implemented the same way, and that is why
it was thrown out leaving only the well-working method developed by Ryoichi
Suzuki.  Sadly, we had committed to the parametric object before looking at
the code and it was hard to get into a even remotely working state fairly
late in the development cycle... :-(

    Thorsten

____________________________________________________
Thorsten Froehlich
e-mail: mac### [at] povrayorg

I am a member of the POV-Ray Team.
Visit POV-Ray on the web: http://mac.povray.org

Post a reply to this message