Subject: Re: CVS hippo: FvwmTheme: crashes really fast

From: Tim Phipps <[email-masked]>
Date: Wed, 22 Sep 1999 13:23:34 +0100

> On Fri, Sep 17, 1999 at 03:57:39PM +0200, Dominik Vogt wrote:
> > 
> > Do you have some instructions how to produce a crash?
> > 

It only crashes when the first *FvwmThemeColorset line uses "contrast"
and "average" as foreground and background colors. The following crashes
if it is the first config line:

*FvwmThemeColorSet 7 contrast average ygradient 200 4 blue 1 navy 1 blue
1 navy 1 blue

but if "contrast" is replaced by "white" it works. Once one colorset has
been processed it doesn't crash on "contrast".

Cheers,
Tim.
--
Visit the official FVWM web page at <URL:http://www.fvwm.org/>.
To unsubscribe from the list, send "unsubscribe fvwm-workers" in the
body of a message to [email-masked].
To report problems, send mail to [email-masked]

---

From: Brad Giaccio <[email-masked]>
Date: Wed, 22 Sep 1999 08:31:52 -0400

> been processed it doesn't crash on "contrast".

But how does that explain my crash on 
*FvwmThemeColorset 0 gray80 #515178 CGradient 128 #515178 #000000
??
Brad
 
-- 
--- There are two kinds of knowledge, you either know the answer or
			you know where to find it
		      -Kane, Johnson, and anonymous
--
Visit the official FVWM web page at <URL:http://www.fvwm.org/>.
To unsubscribe from the list, send "unsubscribe fvwm-workers" in the
body of a message to [email-masked].
To report problems, send mail to [email-masked]

---

From: Tim Phipps <[email-masked]>
Date: Wed, 22 Sep 1999 13:48:12 +0100

> *FvwmThemeColorset 0 gray80 #515178 CGradient 128 #515178 #000000

It doesn't. I was hoping that someone with purify could see the crash
and find out where the memory corruption is happening.

Cheers,
Tim.

---

From: Dominik Vogt <[email-masked]>
Date: Thu, 23 Sep 1999 00:02:11 +0200

> > been processed it doesn't crash on "contrast".

*Sigh*, no, that doesn't cause a crash for me. FvwmTheme didn't
crash on my machine for weeks. Can you send me a minimal
.fvwm2rc that causes a crash?

> *FvwmThemeColorset 0 gray80 #515178 CGradient 128 #515178 #000000

I suspect that this is related to the IconMan crash I got. In this
case it must be in the code that is shared between FvwmTheme and
the other modules, i.e. in libs/Graphics.c.

Bye

Dominik ^_^

--
Dominik Vogt, [email-masked]
Reply-To: [email-masked]
--
Visit the official FVWM web page at <URL:http://www.fvwm.org/>.
To unsubscribe from the list, send "unsubscribe fvwm-workers" in the
body of a message to [email-masked].
To report problems, send mail to [email-masked]

---

From: Dominik Vogt <[email-masked]>
Date: Thu, 23 Sep 1999 01:19:49 +0200

> been processed it doesn't crash on "contrast".

Tim, I have fixed a memory leak in FvwmTheme.c and a call where
potentially random colours could have been free'd. (the address
of the bg member of the colorset struct was used as an array of
three colours). But I don't believe that fixes any crashes.

Bye

Dominik ^_^

---

From: Tim Phipps <[email-masked]>
Date: Thu, 23 Sep 1999 13:14:15 +0100

> the other modules, i.e. in libs/Graphics.c.

I found one memory corruption, the fix is commited. It was in
FvwmTheme.c and was a malloc() that was too small for the following
sprintf().

Brad could you test it?

Cheers,
Tim.

---

From: Brad Giaccio <[email-masked]>
Date: Thu, 23 Sep 1999 09:00:13 -0400

> FvwmTheme.c and was a malloc() that was too small for the following

Well the good news is it no longer crashes, good find.  The even better 
news is I managed to get purify up on my 2.7 box. The bad news is there 
is one memory leak left and I'm not sure what to do                     

The XPutPixel has an array bounds write problem. And I think your
problem and correct me if I'm wrong in Graphics.c you have the following
on line 690 
image->data = safemalloc(image->bytes_per_line * t_height);
shoudln't it be 
image->data = safemalloc(image->bytes_per_line * t_height * t_width);
or am I just terriblly confused?

    case C_GRADIENT:
      {
    register int w = t_width - 1;
    register int h = t_height - 1;
    register int t_scale = (double)(w * h) / sqrt(8);
    
    for (i = 0; i <= w; i++)
      for (j = 0; j <= h; j++) {
            register double x = (i - w / 2) * h / 2;
            register double y = (h / 2 - j) * w / 2;
            register double rad = sqrt(x * x + y * y);
        XPutPixel(image, i, j, pixels[(int)(rad * ncolors / t_scale)]);
      }
    break;  
      }

---

From: Dominik Vogt <[email-masked]>
Date: Thu, 23 Sep 1999 15:34:16 +0200

On Thu, Sep 23, 1999 at 09:00:13AM -0400, [email-masked] wrote:
> The XPutPixel has an array bounds write problem.

Please post the purify output so I can have a look at the problem.

> And I think your

Yes, I gues you *are* terribly confused :-) The size is calculated
with bytes per >line<, not bytes per >pixel<.

>         XPutPixel(image, i, j, pixels[(int)(rad * ncolors / t_scale)]);

Perhaps a rounding error in "rad * ncolors / t_scale"?

Bye

Dominik ^_^

-- 
Dominik Vogt, Hewlett-Packard GmbH, Dept. BVS
Herrenberger Str.130, 71034 Boeblingen, Germany
phone: 07031/14-4596, fax: 07031/14-3883, [email-masked]
--
Visit the official FVWM web page at <URL:http://www.fvwm.org/>.
To unsubscribe from the list, send "unsubscribe fvwm-workers" in the
body of a message to [email-masked].
To report problems, send mail to [email-masked]

---

From: Brad Giaccio <[email-masked]>
Date: Thu, 23 Sep 1999 10:55:30 -0400

> Perhaps a rounding error in "rad * ncolors / t_scale"?

You're right of course I just looked at the log (which should be
attached) and it was array bounds read I was concentrating on array
write problems.  So it probabbly is a rounding error but we can just
check that value < ncolors-1 instead.

---

From: Dominik Vogt <[email-masked]>
Date: Sun, 26 Sep 1999 03:46:05 +0200

> check that value < ncolors-1 instead.

The 'rounding error' theory was nonsens, of course. A rounding error
would cause one value *less* to be read, not one *more*. You would
have to add up x.000001 (or so) quite often before you got a result
that is one higher than expected. It is much worse. The calculation
of x and y is screwed up:

Because 'w' is an integer, 'w / 2' is rounded down if w is odd.
I have changed this to 

  register double x = (double)((2 * i - w) * h) / 4.0;

which should solve the problem. Can you verify this, Brad?
BTW, R and Y gradients had the same problem, but it didn't
cause an array overflow.

Bye

Dominik ^_^

---

From: Danek Duvall <[email-masked]>
Date: Mon, 27 Sep 1999 00:56:22 -0700

>   register double x = (double)((2 * i - w) * h) / 4.0;

Why so complicated?  Shouldn't

    register double x = (i - w / 2.0) * h / 2.0;

do the trick, what with automatic promotion and everything?  It's also a
lot easier to read ...

Danek

---

From: Dominik Vogt <[email-masked]>
Date: Mon, 27 Sep 1999 12:06:22 +0200

> do the trick, what with automatic promotion and everything?  It's also a

I never trust automatic conversions :-) It doesn't really matter
since nobody will understand what this code (plus context) does
in less than half an hour anyway.  I wrote it myself.  I don't
understand it. ;-)

Bye

Dominik ^_^

---

From: Brad Giaccio <[email-masked]>
Date: Mon, 27 Sep 1999 09:18:52 -0400

> BTW, R and Y gradients had the same problem, but it didn't

All right since I'm not sure exactlly what is going on here here are my
numbers could you take a look, I'm still getting the aroun bound read in
the same place.

Using 
FvwmCommand \
"*FvwmThemeColorset 0 gray80 #515178 CGradient 128 #515178 #000000"

I get the following 4 around bound read. Maybe you can spot what went
wrong, it sounds like you are a little more farmilliar with the code
here.

i = 0 j = 0 w = 254 h = 254 x = -16129.000000 y = 16129.000000 rad = 22809.850548 index = 128.004773 128
i = 0 j = 254 w = 254 h = 254 x = -16129.000000 y = -16129.000000 rad = 22809.850548 index = 128.004773 128
i = 254 j = 0 w = 254 h = 254 x = 16129.000000 y = 16129.000000 rad = 22809.850548 index = 128.004773 128
i = 254 j = 254 w = 254 h = 254 x = 16129.000000 y = -16129.000000 rad = 22809.850548 index = 128.004773 128

---

From: Dominik Vogt <[email-masked]>
Date: Mon, 27 Sep 1999 15:37:40 +0200

> i = 254 j = 254 w = 254 h = 254 x = 16129.000000 y = -16129.000000 rad = 22809.850548 index = 128.004773 128

Looks right this far. It is important that the x//y values with i//j = 0 are the same
as with i//j = w//h (except for the sign). This is the case in abouve output:
-(16129) = -16129. What are the values of t_scale, ncolors and myncolors?
Theoretically rad = 22809.850548 should be equal to t_scale*0.5.

Bye

Dominik ^_^

---

From: Brad Giaccio <[email-masked]>
Date: Mon, 27 Sep 1999 11:57:26 -0400

> Theoretically rad = 22809.850548 should be equal to t_scale*0.5.

well t_scale = (double)(w * h) / sqrt(8); or in my case 22809.850547516
and ncolors = 128, myncolors isn't defined for CGradient.

It seems we just have a simple off by one here.  And one of us is not
seeing it, looking at Graphics.c:679 we have 
	XPutPixel(image, i, j, pixels[(int)(rad * ncolors / t_scale)]); 

now we are indexing into an array of size 0 to ncolors -1 in order for
that to work rad / t_scale must be < 1.  In the cases above it is not
Now if we made t_scale a double we would get rad/t_scale == 1 but 1 *
ncolors > ncolors -1.

Now some where we need to subtract one to get our array index correct.
I'm just not sure where we need to make the change.

Brad

---

From: Dominik Vogt <[email-masked]>
Date: Mon, 27 Sep 1999 18:28:34 +0200

> Now if we made t_scale a double we would get rad/t_scale == 1 but 1 *

Oops, seems I forgot to commit a '-epsilon' I had already planned:
Correct version should be:

  XPutPixel(image, i, j, pixels[(int)((rad * ncolors - 0.5) / t_scale)]);
                                     ^^^            ^^^^^^^^
> Now some where we need to subtract one to get our array index correct.

No, with -1 we may run into trouble with very few colours.

> I'm just not sure where we need to make the change.

Never mind, I'll commit the change in a few minutes.

Bye

Dominik ^_^