Bug 10320 - erand48 implementation not thread safe but POSIX says it should be
Summary: erand48 implementation not thread safe but POSIX says it should be
Status: REOPENED
Alias: None
Product: glibc
Classification: Unclassified
Component: libc (show other bugs)
Version: 2.11
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-06-23 19:25 UTC by Simon Josefsson
Modified: 2014-07-01 07:56 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:
fweimer: security-


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Josefsson 2009-06-23 19:25:36 UTC
POSIX specification for erand48:

http://www.opengroup.org/onlinepubs/9699919799/functions/erand48.html

It doesn't say the function may be non-reentrant, which IIUC, means it should be
thread safe.  However the implementation of erand48 does use a global variable:

http://sourceware.org/git/?p=glibc.git;a=blob;f=stdlib/erand48.c;hb=HEAD

The variable is __libc_drand48_data and is defined in:

http://sourceware.org/git/?p=glibc.git;a=blob;f=stdlib/drand48-iter.c;hb=HEAD

I can't find anyplace where it is made thread-local, so I'm assuming it is a
real global variable.

As far as I can tell, there is no requirement in the specification that erand48
update the global drand48 state.
Comment 1 Simon Josefsson 2009-06-23 20:10:57 UTC
By reading code, jrand48, lcong48, nrand48, seed48, and srand48 also uses the
variable __libc_drand48_data so may be subjec to the same concern.
Comment 2 Ondrej Bilka 2012-12-19 21:41:08 UTC
On

http://www.opengroup.org/onlinepubs/9699919799/functions/erand48.html

change history shows that note on nonreentrancy was added in meantime:

Issue 5

A note indicating that the drand48(), lrand48(), and mrand48() functions need not be reentrant
Comment 3 Simon Josefsson 2012-12-20 08:21:28 UTC
As noted jrand48, lcong48, nrand48, seed48, and srand48 also uses the global variable.  The POISX description does not permit those functions to be thread unsafe.

The title of this bug report could thus be changed to "jrand48, lcong48, nrand48, seed48, and srand48 implementations not thread safe but POSIX says it should be" but I don't see how to change the title in bugzilla.  If you want me to open a new bug instead, let me know.

/Simon
Comment 4 Ondrej Bilka 2012-12-20 12:11:58 UTC
Sorry that I mistaken erand48 for drand48.

Note that there is a GNU extension reentrant functions drand48_r ...

Function lcong48 changes a,c in x_{n+1}=a*x_n+c mod 2^48 congruence formula. 
These are saved in __libc_drand48_data

Then erand48... read only these a,c from __libc_drand48_data.

There is indeed a race between lcong48 and *rand48 but it is mostly harmless.

Only thing that can go wrong is that when lcong48 with a',c' is called then some seed will be updated according to formula x_{n+1}=a'*x_n+c . As then seed is nondeterministic because both x_{n+1}=a*x_n+c , x_{n+1}=a'*x_n+c' are valid updates.
Comment 5 Rich Felker 2012-12-20 22:52:25 UTC
Can you clarify whether there is actually a bug here? POSIX says:

  The drand48(), lrand48(), and mrand48() functions need not be thread-safe.

If you're claiming erand48, jrand48, and/or nrand48 also have thread-safety problems, please clarify that and explain what the problem is.
Comment 6 Simon Josefsson 2012-12-21 08:12:50 UTC
(In reply to comment #5)
> Can you clarify whether there is actually a bug here? POSIX says:
> 
>   The drand48(), lrand48(), and mrand48() functions need not be thread-safe.
> 
> If you're claiming erand48, jrand48, and/or nrand48 also have thread-safety
> problems, please clarify that and explain what the problem is.

The problem should be completely described if you read the first two posts in this report.

To summarize the current state:

* drand48, lrand48, mrand48 need not be thread-safe.
* erand48, jrand48, lcong48, nrand48, seed48, and srand48 ought to be thread-safe, as far as I understand.
* The erand48, jrand48, lcong48, nrand48, seed48, and srand48 functions appears to be thread unsafe in glibc, since they use a global variable.

The links into git posted in the first post (3 years ago) still works, if you want to read the code.

/Simon
Comment 7 Rich Felker 2012-12-21 20:35:33 UTC
I think what you're saying is that srand48, seed48, and lcong48 are modifying data in non-thread-safe ways, and that erand48, jrand48, and nrand48 are using some of that data (the part set by lconf48) in non-thread-safe ways. Is this correct?

The issue seems rather minor to me. srand48 and seed48 have no place in multi-threaded usage at all; they only set the state for functions which are not thread-safe anyway. lcong48 does set state that's used by the thread-safe functions, but even if it did modify this state in a thread-safe way, calling it at the same time random-number-generating functions are in use would have unpredictable results.

Considering that it would be prohibitively expensive to make the thread-safe random number interfaces perform locking to access the state set by lcong48, I think the right approach to this issue is to open a bug report with the Austin Group. It seems to me that failure to list srand48, seed48, and lcong48 as non-thread-safe was just a mistake, not an intentional feature requirement on implementations.
Comment 8 Jackie Rosen 2014-02-16 17:45:40 UTC Comment hidden (spam)