This is the mail archive of the
libc-alpha@sources.redhat.com
mailing list for the glibc project.
[fischer: Re: Multiple bugs in stdlib function setstate()]
- To: libc-alpha at sourceware dot cygnus dot com
- Subject: [fischer: Re: Multiple bugs in stdlib function setstate()]
- From: Michael Fischer <fischer at cs dot yale dot edu>
- Date: Sun, 20 Aug 2000 17:53:00 -0400 (EDT)
[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 ****