[fischer: Re: Multiple bugs in stdlib function setstate()]

[Michael Fischer <fischer at cs dot yale dot edu>]

[Forgot to cc the libc-alpha mailing list on my earlier reply. --Mike]

**** Forwarded Message Follows ****
Date: 20 Aug 2000 15:01:09 -0400 (Sun)
From: fischer
Subject: Re: Multiple bugs in stdlib function setstate()
To: drepper@cygnus.com (Ulrich Drepper)
In-Reply-To: Ulrich Drepper <drepper@redhat.com>, 20 Aug 2000 09:57:06 -0700

Dear Ulrich,

Sorry for not knowing where to look for the latest test release of
glibc.  I've downloaded version 2.1.92 now.  My bug #1 has already
been fixed.  Here are further comments on the other two bugs I
reported:

    > 2.  setstate(newstate) corrupts new state, causing subsequent
    >     setstate(newstate) call to fail (returns NULL).
    
    That's how the functions are written.

NOT FIXED!!!  The old BSD version of this function was correct.  A bug
was introduced in making the code reentrant.  I explained this bug in
a note in my bug report:

    Discussion: The problem is the +1 offset between buf->state and
    new_state.  buf->rptr and buf->fptr should end up pointing to
    buf->state[rear] and buf->state[(rear + separation) % degree].
    However, &buf->state[0] = &new_state[1], so &buf->state[k] =
    &new_state[1 + k]; hence the need to add 1 to the two subscripts
    above.  (Other fixes are of course also possible.)

I also included a test program that exposes the bug.

Here are the relevant lines from the old BSD code, version 5.9
(Berkeley) 2/23/91:

	state = &new_state[1];
	if (rand_type != TYPE_0) {
		rptr = &state[rear];
		fptr = &state[(rear + rand_sep) % rand_deg];
	}
	end_ptr = &state[rand_deg];		/* set end_ptr too */

In glibc-2.1.92, they have become:

  if (type != TYPE_0)
    {
      int rear = new_state[0] / MAX_TYPES;
      buf->rptr = &new_state[rear];
      buf->fptr = &new_state[(rear + separation) % degree];
    }
  buf->state = &new_state[1];
  /* Set end_ptr too.  */
  buf->end_ptr = &new_state[degree];

In the old code, "state" was set to &new_state[1] BEFORE setting rptr
and fptr.  Thus, setting rptr = &state[rear] is equivalent to setting
rptr = &new_state[1+rear], and similarly for fptr.

In the new code, rptr and fptr are set directly from new_state, but the
subscripts were not adjusted accordingly.  That is the bug.

The effect of the bug is that sometimes rptr or fptr ends up pointing
to state[0] (which it never should).  Things gradually deteriorate
from there.  After awhile state[0] gets corrupted.  Then setstate()
fails when that state is reinstalled and returns NULL.  A subsequent
call of setstate on that returned value causes a segmentation fault.
All this because of a +1 error in a couple of subscripts!

-------

    > 3.  setstate(NULL) causes segmentation fault.
    
    And your problem is?  We are never testing for invalid arguments
    unless it is demanded by a standard.  Which is not the case here.

My problem is just one of consistency.  __setstate_r() already sets
errno to EINVAL under various conditions.  But maybe they're the only
ones that are demanded by the standards.

--Mike

==================================================
| Michael Fischer <fischer-michael@cs.yale.edu>  |
| Professor of Computer Science                  |
==================================================

**** End of Forwarded Message ****