Bug 192801

Jan, could you provide a minimal working example that shows the crash, and example that fixes the problem.

Created attachment 252891 [details]
rng.c

Run under Valgrind to see uninitialized values.

(In reply to Michael Kerrisk from comment #1)
> Jan, could you provide a minimal working example that shows the crash, and
> example that fixes the problem.

It would be relatively too time consuming to extract code that crashes. I believe that valgrind output is sufficient to demonstrate that initialization is necessary:

$ gcc -g -o rng rng.c
$ valgrind ./rng
==12756== Memcheck, a memory error detector
==12756== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==12756== Using Valgrind-3.12.0 and LibVEX; rerun with -h for copyright info
==12756== Command: ./rng
==12756== 
==12756== Conditional jump or move depends on uninitialised value(s)
==12756==    at 0x3904235D0B: initstate_r (random_r.c:242)
==12756==    by 0x400683: main (rng.c:21)
==12756== 
==12756== Conditional jump or move depends on uninitialised value(s)
==12756==    at 0x3904235D14: initstate_r (random_r.c:245)
==12756==    by 0x400683: main (rng.c:21)
==12756== 
==12756== Use of uninitialised value of size 8
==12756==    at 0x3904235E28: initstate_r (random_r.c:248)
==12756==    by 0x400683: main (rng.c:21)
==12756== 
-900787106

Replacing the first "if(0) bzero..." in rng.c with "if(1) bzero..." fixes the uninitialized values.

(In reply to Jan Ziak (http://atom-symbol.net) from comment #0)

Thanks for the example code. That was helpful.

> ----
> 
> Suggested wording:
> 
> The arguments statebuf and buf must be initialized to zero via for example
> memset() or bzero(). Passing uninitialized (not zeroed) statebuf or buf to
> initstate_r() results in undefined behavior.

So, my reading of th the glibc source 9admittedly not too deeply) and a little playing with your code suggests that the only thing that needs to be initialized is:

    buf.state = NULL;

But, I may have missed something. Why do you think everything else must be zeroed?

> (In reply to Jan Ziak (http://atom-symbol.net) from comment #0)
> 
> Thanks for the example code. That was helpful.
> 

For example, increasing sizeof(rng.state) from 8 to 80 will produce a crash:

==8414== Process terminating with default action of signal 11 (SIGSEGV): dumping core
==8414==  General Protection Fault
==8414==    at 0x3904235E28: initstate_r (random_r.c:248)
==8414==    by 0x400692: main (rng.c:21)

> > Suggested wording:
> > 
> > The arguments statebuf and buf must be initialized to zero via for example
> > memset() or bzero(). Passing uninitialized (not zeroed) statebuf or buf to
> > initstate_r() results in undefined behavior.
> 
> So, my reading of th the glibc source 9admittedly not too deeply) and a
> little playing with your code suggests that the only thing that needs to be
> initialized is:
> 
>     buf.state = NULL;

True. statebuf does not need to be initialized.

In my opinion, the structure random_data should be treated as opaque to the user of glibc. memset() seems more appropriate than buf.state=NULL.

> But, I may have missed something. Why do you think everything else must be
> zeroed?

I didn't take a close look at the implementation of __srandom_r() which initializes buf.state[i] and __random_r() which uses buf.state.

(In reply to Jan Ziak (http://atom-symbol.net) from comment #5)

> > > Suggested wording:
> > > 
> > > The arguments statebuf and buf must be initialized to zero via for
> example
> > > memset() or bzero(). Passing uninitialized (not zeroed) statebuf or buf
> to
> > > initstate_r() results in undefined behavior.
> > 
> > So, my reading of th the glibc source 9admittedly not too deeply) and a
> > little playing with your code suggests that the only thing that needs to be
> > initialized is:
> > 
> >     buf.state = NULL;
> 
> True. statebuf does not need to be initialized.
> 
> In my opinion, the structure random_data should be treated as opaque to the
> user of glibc. memset() seems more appropriate than buf.state=NULL.

I understand what you mean. It feels like one should treat it as opaque data. But, on the other hand, zeroing out that field using memset() isn't strictly correct according to the C standard, since NULL need not be the same as 0 (although it is in all sane implementations). So,at the moment, I'm still inclined to document setting buf.state to NULL rather than the use of memset().

Looking in the glibc source file string/strfry.c, I see:

  if (!init)
    {
      static char state[32];
      rdata.state = NULL;
      __initstate_r (time ((time_t *) NULL) ^ getpid (),
                     state, sizeof (state), &rdata);
      init = 1;
    }

So, it seems that initializing the 'state' field is what the glibc maintainer of the time thought was the right approach.

Also relevant:
https://sourceware.org/bugzilla/show_bug.cgi?id=3662

I've now made a number of changes to the random_r(3) page, as summarized in the diff below. Closing this bug now. Please reopen if you thing simething still needs to be addressed.

diff --git a/man3/random_r.3 b/man3/random_r.3
index 12cee1a..e4d4798 100644
--- a/man3/random_r.3
+++ b/man3/random_r.3
@@ -94,6 +94,26 @@ function is like
 except that it initializes the state in the object pointed to by
 .IR buf ,
 rather than initializing the global state variable.
+Before calling this function, the
+.IR buf.state
+field must be initialized to NULL.
+The
+.BR initstate_r ()
+function records a pointer to the
+.I statebuf
+argument inside the structure pointed to by
+.IR buf .
+Thus,
+.IR statebuf
+should not be deallocated so long as
+.IR buf
+is still in use.
+(So,
+.I statebuf
+should typically be allocated as a static variable,
+or allocated on the heap using
+.BR malloc (3)
+or similar.)
 
 The
 .BR setstate_r ()
@@ -102,6 +122,11 @@ function is like
 except that it modifies the state in the object pointer to by
 .IR buf ,
 rather than modifying the global state variable.
+\fIstate\fP must first have been initialized
+using
+.BR initstate_r ()
+or be the result of a previous call of
+.BR setstate_r ().
 .SH RETURN VALUE
 All of these functions return 0 on success.
 On error, \-1 is returned, with
@@ -150,6 +175,17 @@ T}	Thread safety	MT-Safe race:buf
 These functions are nonstandard glibc extensions.
 .\" These functions appear to be on Tru64, but don't seem to be on
 .\" Solaris, HP-UX, or FreeBSD.
+.SH BUGS
+The
+.BR initstate_r ()
+interface is confusing.
+.\" FIXME . https://sourceware.org/bugzilla/show_bug.cgi?id=3662
+It appears that the
+.IR random_data
+type is intended to be opaque,
+but the implementation requires the user to either initialize the
+.I buf.state
+file to NULL or zero out the entire structure before the call.
 .SH SEE ALSO
 .BR drand48 (3),
 .BR rand (3),

(In reply to Michael Kerrisk from comment #9)
> I've now made a number of changes to the random_r(3) page, as summarized in
> the diff below. Closing this bug now. Please reopen if you thing simething
> still needs to be addressed.

The diff looks good. Thanks.

> diff --git a/man3/random_r.3 b/man3/random_r.3
> index 12cee1a..e4d4798 100644
> --- a/man3/random_r.3
> +++ b/man3/random_r.3
> @@ -102,6 +122,11 @@ function is like
>  except that it modifies the state in the object pointer to by

Unrelated to this bug:

There seems to be a typo: "object pointeR to by" -> "object pointeD to by".

(In reply to Jan Ziak (http://atom-symbol.net) from comment #10)
> (In reply to Michael Kerrisk from comment #9)
> > I've now made a number of changes to the random_r(3) page, as summarized in
> > the diff below. Closing this bug now. Please reopen if you thing simething
> > still needs to be addressed.
> 
> The diff looks good. Thanks.

Thanks for checking it over.

> > diff --git a/man3/random_r.3 b/man3/random_r.3
> > index 12cee1a..e4d4798 100644
> > --- a/man3/random_r.3
> > +++ b/man3/random_r.3
> > @@ -102,6 +122,11 @@ function is like
> >  except that it modifies the state in the object pointer to by
> 
> Unrelated to this bug:
> 
> There seems to be a typo: "object pointeR to by" -> "object pointeD to by".

Thanks. Fixed!