divide by zero in random/taviso()

divide by zero in random/taviso()

[Vince Weaver]

Hello

so for my perf_fuzzer I re-use the trinity random.c, and today I hit

[ 1241.026870] traps: perf_fuzzer[4509] trap divide error ip:40a779 

which maps to the taviso() routine which is doing:

           case 1: r = rand() % rand();
#if __WORDSIZE == 64
                r <<= 32;
                r |= rand() % rand();
#endif
                break;

which is a divide by zero if rand() returns 0.

Is there a good way to fix this short of a lot of special case checks?

Vince Weaver
vincent.weaver@maine.edu
http://www.eece.maine.edu/~vweaver/

[patch] fix divide by zero in random/taviso()

[Vince Weaver]

I'm not sure if this is the cleanest fix for the issue, but it seems to 
work for me.

In random.c the taviso() routine uses "rand() % rand()" which
can cause a floating point exception if rand() returns 0.
Add some tests to make sure that doesn't happen.

Signed-off-by: Vince Weaver <vincent.weaver@maine.edu>

diff --git a/random.c b/random.c
index 7def9ff..a71fcea 100644
--- a/random.c
+++ b/random.c
@@ -42,6 +42,7 @@ static unsigned long randbits(int limit)
 static unsigned long taviso(void)
 {
 	unsigned long r = 0;
+	unsigned long temp;
 
 	switch (rand() % 4) {
 	case 0:	r = rand() & rand();
@@ -51,10 +52,14 @@ static unsigned long taviso(void)
 #endif
 		break;
 
-	case 1:	r = rand() % rand();
+	case 1:	temp = rand();
+		r = rand();
+		if (!temp) r %= temp;
 #if __WORDSIZE == 64
 		r <<= 32;
-		r |= rand() % rand();
+
+		temp = rand();
+		if (!temp) r |= rand() % temp;
 #endif
 		break;
 

Re: [patch] fix divide by zero in random/taviso()

[Dave Jones]

On Fri, Jan 17, 2014 at 03:41:58PM -0500, Vince Weaver wrote:

 > I'm not sure if this is the cleanest fix for the issue, but it seems to 
 > work for me.
 
Good enough.

 > In random.c the taviso() routine uses "rand() % rand()" which
 > can cause a floating point exception if rand() returns 0.
 > Add some tests to make sure that doesn't happen.
 > 
 > Signed-off-by: Vince Weaver <vincent.weaver@maine.edu>
 > 
 > diff --git a/random.c b/random.c
 > index 7def9ff..a71fcea 100644
 > --- a/random.c
 > +++ b/random.c
 > @@ -42,6 +42,7 @@ static unsigned long randbits(int limit)
 >  static unsigned long taviso(void)
 >  {
 >  	unsigned long r = 0;
 > +	unsigned long temp;
 >  
 >  	switch (rand() % 4) {
 >  	case 0:	r = rand() & rand();
 > @@ -51,10 +52,14 @@ static unsigned long taviso(void)
 >  #endif
 >  		break;
 >  
 > -	case 1:	r = rand() % rand();
 > +	case 1:	temp = rand();
 > +		r = rand();
 > +		if (!temp) r %= temp;
 >  #if __WORDSIZE == 64
 >  		r <<= 32;
 > -		r |= rand() % rand();
 > +
 > +		temp = rand();
 > +		if (!temp) r |= rand() % temp;
 >  #endif
 >  		break;

Good catch.  Despite the function name, this is something I introduced,
not something that was in Tavis' original code in iknowthis.

Mea culpa.

	Dave

 

Re: [patch] fix divide by zero in random/taviso()

[Dave Jones]

On Tue, Jan 21, 2014 at 11:03:43AM -0800, Andrew Honig wrote:
 
 > >  > In random.c the taviso() routine uses "rand() % rand()" which
 > >  > can cause a floating point exception if rand() returns 0.
 > >  > Add some tests to make sure that doesn't happen.
 > >  >
 > >  > Signed-off-by: Vince Weaver <vincent.weaver@maine.edu>
 > >  >
 > >  > diff --git a/random.c b/random.c
 > >  > index 7def9ff..a71fcea 100644
 > >  > --- a/random.c
 > >  > +++ b/random.c
 > >  > @@ -42,6 +42,7 @@ static unsigned long randbits(int limit)
 > >  >  static unsigned long taviso(void)
 > >  >  {
 > >  >      unsigned long r = 0;
 > >  > +    unsigned long temp;
 > >  >
 > >  >      switch (rand() % 4) {
 > >  >      case 0: r = rand() & rand();
 > >  > @@ -51,10 +52,14 @@ static unsigned long taviso(void)
 > >  >  #endif
 > >  >              break;
 > >  >
 > >  > -    case 1: r = rand() % rand();
 > >  > +    case 1: temp = rand();
 > >  > +            r = rand();
 > >
 > 
 > Perhaps, I'm reading this wrong, but this looks backwards to me.  This will
 > perform the mod operation only when temp is 0.  Shouldn't this be
 > if (temp) r %= temp;

clang seems to agree.

random.c:57:16: warning: Division by zero
                if (!temp) r %= temp;
                           ~~^~~~~~~
random.c:62:26: warning: Division by zero
                if (!temp) r |= rand() % temp;
                                ~~~~~~~^~~~~~

Vince, want to send a follow-up fix ?

	Dave

Re: [patch] fix divide by zero in random/taviso()

[Vince Weaver]

On Tue, 21 Jan 2014, Dave Jones wrote:

> random.c:57:16: warning: Division by zero
>                 if (!temp) r %= temp;
>                            ~~^~~~~~~
> random.c:62:26: warning: Division by zero
>                 if (!temp) r |= rand() % temp;
>                                 ~~~~~~~^~~~~~
> 
> Vince, want to send a follow-up fix ?

Yes, brown-paper bag time.  Never trust "it compiles and runs" as 
testing for a bug that only shows up 1 in 4 billion times anyway.

I've copied the code into a test harness and actually ran it with code 
that returns zero more often to verify it works (after hacking things 
so gcc didn't "helpfully" optimize the zero generating code away).

---

The previous fix to the divide-by-zero bug in random/taviso()
(fea56c28830f "fix divide by zero in random/taviso()")
had the zero-checks inverted.  This meant the old div-by-zero bug
was still there, and worse, it meant that the number being generated
always had 32-bits of zeros in the bottom half on a 64-bit machine.

Signed-off-by: Vince Weaver <vincent.weaver@maine.edu>

diff --git a/random.c b/random.c
index 75fa868..38ed22f 100644
--- a/random.c
+++ b/random.c
@@ -54,12 +54,12 @@ static unsigned long taviso(void)
 
 	case 1:	temp = rand();
 		r = rand();
-		if (!temp) r %= temp;
+		if (temp) r %= temp;
 #if __WORDSIZE == 64
 		r <<= 32;
 
 		temp = rand();
-		if (!temp) r |= rand() % temp;
+		if (temp) r |= rand() % temp;
 #endif
 		break;