[Bug libc/3662] New: Implementation bugs in random_r and friends

[Bug libc/3662] New: Implementation bugs in random_r and friends

[glibcbugs0000 at cneufeld dot ca]

There are at least two implementation bugs in the random_r class of functions.

First, the random_data structure, being opaque, is typically just allocated on
the stack or malloc()ed.  There is no way to create a valid "this is not an old
state vector" structure without knowing the implementation details of the
structure.  If the compiler does not initialize the "state" member of the
structure to a value equal to the NULL pointer, the initstate_r() function will
crash as it dereferences whatever non-NULL but invalid value is held there.  We
need a method to create a blank random_data object, or the documentation has to
point out that the user is responsible for initializing the "state" member to NULL.

Second, initstate_r() and setstate_r() are documented in their comment blocks as
returning a pointer to the old state, but they do not.  There is no documented
way to retrieve the old state in a fashion that allows it to be re-introduced
into the system with setstate_r() at a later time.  As such, the setstate_r()
function is essentially unusable.

-- 
           Summary: Implementation bugs in random_r and friends
           Product: glibc
           Version: 2.4
            Status: NEW
          Severity: normal
          Priority: P2
         Component: libc
        AssignedTo: drepper at redhat dot com
        ReportedBy: glibcbugs0000 at cneufeld dot ca
                CC: glibc-bugs at sources dot redhat dot com


http://sourceware.org/bugzilla/show_bug.cgi?id=3662

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug libc/3662] Implementation bugs in random_r and friends

[drepper at redhat dot com]

------- Additional Comments From drepper at redhat dot com  2006-12-05 23:58 -------
If you want to see documentation changes you do the work.

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |SUSPENDED


http://sourceware.org/bugzilla/show_bug.cgi?id=3662

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug libc/3662] Implementation bugs in random_r and friends

[glibcbugs0000 at cneufeld dot ca]

------- Additional Comments From glibcbugs0000 at cneufeld dot ca  2006-12-06 03:02 -------
Created an attachment (id=1448)
 --> (http://sourceware.org/bugzilla/attachment.cgi?id=1448&action=view)
Add documentation necessary to use initstate_r() and setstate_r()

This fixes the incorrect comments in the .c file and adds necessary details to
the .texi file allowing the user to make use of initstate_r() and setstate_r(),
assuming that the implementation of struct random_data remains stable.

-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=3662

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug libc/3662] Implementation bugs in random_r and friends

[kenta at cs dot stanford dot edu]

------- Additional Comments From kenta at cs dot stanford dot edu  2009-11-25 03:24 -------
I hit this bug today.  Can the patch be deployed?

(What does the SUSPENDED but status mean?)

-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=3662

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug libc/3662] Implementation bugs in random_r and friends

[kenta at cs dot stanford dot edu]

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |kenta at cs dot stanford dot
                   |                            |edu


http://sourceware.org/bugzilla/show_bug.cgi?id=3662

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug libc/3662] Implementation bugs in random_r and friends

[drepper at redhat dot com]

------- Additional Comments From drepper at redhat dot com  2009-11-25 04:35 -------
The user should always initialize the entire structure with NUL bytes.  No need
to know the members of the structure, just the size is needed.  It shouldn't be
documented that looking at the members is needed.

If you update the patch change the state of the bug.  I don't see suspended bugs
in my queries.

I have applied the patch to random_r.c.

-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=3662

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug libc/3662] Implementation bugs in random_r and friends

[glibcbugs0000 at cneufeld dot ca]

------- Additional Comments From glibcbugs0000 at cneufeld dot ca  2009-11-25 12:20 -------
Changing the comments isn't really going to fix the issue.  The problem is that
the random_r class of functions are entirely broken to people who do not know
the implementation details of the random_data structure.

For instance, without knowing the innards of random_data, it's impossible to
call srandom_r() without getting a segmentation violation (try it!).  The
random_data structure cannot be just set to all bytes zero, because the "state"
member of it has to point to a valid external buffer.  srandom_r() can't malloc
this itself because there's no destructor-equivalent to release the data when
we're finished (performing the role that regfree() does in the regex code).

So, without srandom_r(), all you're left with is initstate_r().  This function
will work if you hand it a random_data structure set to all zeroes, but what the
documentation doesn't tell you is that it then modifies something inside
random_data to point to the buffer you passed as state.  This is deadly, it
means if the buffer you created to hold the incoming state was an auto variable,
the random_r functions will be using released stack space as their internal
scratch space, merrily stomping on stack memory every time the user asks for
another random number.

So, without knowing the internal details of random_data, the only way to start
up the random number generator is with initstate_r(), in which case it is
critically important (but undocumented) that the second argument be of static
duration and valid throughout the life of the program.  Even then, there is no
documented way to call setstate_r() correctly.

I would suggest that the random_data structure should be redesigned.  Rather
than "state" being a pointer to an external entity, the entire state vector
should be stored within random_data.  In this way, you can safely manipulate it
without wondering whether the memory it points to is valid.

I can supply a patch to do this, if you agree that this is the way to proceed.

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|SUSPENDED                   |NEW


http://sourceware.org/bugzilla/show_bug.cgi?id=3662

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug libc/3662] Implementation bugs in random_r and friends

[drepper at redhat dot com]

------- Additional Comments From drepper at redhat dot com  2009-11-25 14:26 -------
(In reply to comment #5)
> I would suggest that the random_data structure should be redesigned.

I already said no.  The interfaces are used internally and that's really their
only purpose.  Design and implement your own library with randomization
functions if you must.  If you don't want to provide any more patches to the
documentation the bug might as well be closed.

-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=3662

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug libc/3662] Implementation bugs in random_r and friends

[glibcbugs0000 at cneufeld dot ca]

------- Additional Comments From glibcbugs0000 at cneufeld dot ca  2009-11-25 14:50 -------
So, if I understand your objection, you feel that the random_r class of
functions is used for the internal implementation details of the non-reentrant
functions.  You feel that the internal details of random_data should not be
exposed to the users, and that only changes to documentation be used to address
the issues brought up in this bug report.

I am not trying to be difficult here, I'm trying to help.  I will write and
attach a patch that does the following:

1) Remove the documentation for srandom_r() because without an understanding of
the internal implementation of random_data there is literally no way to call
this function without segfaulting.  Instead I will note that this function is
for internal use only and must not be called by the user.

2) Document that initstate_r() must be given a static-duration buffer for its
second argument.  Also document that the previous value of the state information
is not returned by this function, as is currently claimed.

3) Document the way setstate_r() might be usable in the context of
initstate_r(), based on my reading of the source code.

Note that this patch will not fix the random_r.3 manpage, which will continue to
claim that srandom_r() is a usable function.


Here is what the patch will NOT do, but I'd like to record these thoughts
alongside this bug for future reference.  I suggest that if the random_data
structure were augmented with an array of length 256 appended to the end,
correctly aligned for integer access, that the srandom_r() function could be
rescued in a backward-compatible way by making 'state' point to this new
internal buffer.  The initstate_r() function could also be fixed up, still
backwardly compatible with older binaries, with the old state being returned in
the passed buffer.  Further, a random_data structure would now be a
self-contained object that could be written to disk and loaded at a later time,
unlike the current condition where the structure contains a pointer that points
outside itself, to a block of unknown length.

-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=3662

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug libc/3662] Implementation bugs in random_r and friends

[glibcbugs0000 at cneufeld dot ca]

------- Additional Comments From glibcbugs0000 at cneufeld dot ca  2009-11-25 15:20 -------
Created an attachment (id=4415)
 --> (http://sourceware.org/bugzilla/attachment.cgi?id=4415&action=view)
Change documentation to reflect realities of the random_r() group of functions

I've made the changes suggested in my previous comment.  I also came to the
conclusion that setstate_r() isn't usable without a way to get a value back
from initstate_r(), so I've documented that function as also being for
internal-use only.

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
Attachment #1448 is|0                           |1
           obsolete|                            |


http://sourceware.org/bugzilla/show_bug.cgi?id=3662

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug libc/3662] Implementation bugs in random_r and friends

[kenta at cs dot stanford dot edu]

------- Additional Comments From kenta at cs dot stanford dot edu  2010-07-27 07:58 -------
glibc is misspelled "glic" in the patch.

-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=3662

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.