Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 452638 - initstate() causes subtle memory corruption
Summary: initstate() causes subtle memory corruption
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: glibc
Version: 9
Hardware: All
OS: Linux
low
high
Target Milestone: ---
Assignee: Jakub Jelinek
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-06-24 07:56 UTC by JW
Modified: 2008-06-25 23:15 UTC (History)
1 user (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-06-25 15:08:24 UTC


Attachments (Terms of Use)

Description JW 2008-06-24 07:56:12 UTC
Description of problem:
initstate() does the unexpected - it writes to previous state before
initializing new state

Version-Release number of selected component (if applicable):
glibc-2.8-3

How reproducible:
Always

Steps to Reproduce:
1. Compile and run this:
===================================================
#include <stdio.h>
#include <stdlib.h>

int
main()
{
    char *state1 = calloc(128, sizeof(*state1));
    char *state2 = calloc(128, sizeof(*state1));
    initstate(1, state1, 128);
    free(state1);
    initstate(1, state2, 128);
    free(state2);
}
=================================================
2. Run with valgrind 
 
Actual results:
1. Depending on compiler, might produce a segmentation fault
2. Will show invalid write from initstate_r()

Expected results:
1. Should work without memory corruption
2. Should show no errors

Additional info:
Unexpectedly, initstate_r() takes a considerable liberty with its own name and
not only initializes the new state structure but unexpectedly modifies the old
state structure.

A segmentation fault is not guaranteed.  Instead a subtle memory corruption (4
bytes) can occur which may not become apparent until some hapless structure
discovers it has had garbage written to it.

If a program consists of several independently authored modules it is entirely
feasible that there might be multiple entirely valid instances of initstate().

To avoid this error one is supposed to do the following, but it is not
documented anywhere - you have to read the glibc source:

    char *state1 = calloc(64, sizeof(*state1));
    char *oldstate = initstate(1, state1, 64)
    ...
    initstate(1, oldstate, 128); // magic 128 because I have read the source!
    free(state1);

But a modular program is not going to be passing around an oldstate to another
module that might need it.

In summary, initstate_r() should only initialize the new state array, and it
should NOT assume that any old state array still points to valid memory. Or, if
it is generally believed that the old state needs fiddling with before
initstate_r() does it work, then might I suggest that somebody create a clear
method to do so - perhaps uninitstate_r().

Comment 1 Jakub Jelinek 2008-06-25 09:57:58 UTC
I believe it is just a user error to free or otherwise touch the state buffer
which you gave to initstate/setstate until one of the two calls switches
to a different buffer.

Comment 2 JW 2008-06-25 10:26:27 UTC
Where does the documentation say that?

And how is anybody going to structure their programs in such a way that one
module can free the other module's buffer AFTER calling initstate()? And/or how
is another independent module going to know that some other module has called
initstate()? Please explain.

Sometimes I wonder why I bother!


Comment 3 Jakub Jelinek 2008-06-25 12:36:34 UTC
Why would you need to pass oldstate to another module?  If each module wants
to have its own random state, then just use:
char *mystate = calloc (128, 1);
char *oldstate = initstate (1, mystate, 128);
...
use random () within the module

setstate (oldstate);
free (mystate);
in each module.  Note that if it is in library code that might ever be used
from threaded programs, you really want to use the *_r variants.

Comment 4 JW 2008-06-25 13:38:38 UTC
Well, my friend, please explain where the necessity to invoke setstate(oldstate)
is documented?

Do we really need to dig down into glibc source to determine the dire necessity
to invoked setstate(oldstate)?

Where does the documentation state "... you must always invoked
setstate(oldstate) before invoking setstate(newstate) ..."?  Where does it
explain that setstate() will attempt to modify the previous state pointers even
though you thought you were finished with them?

The glibc code could be structured so that the old state doesn't need to be
written to when setting a new state.  Besides, what is the point?  If you are
setting a new state structure why try to do anything to the old one.  The old
state structure is history and should be treated as such.  Once setstate() or
initstate() is invoked any old state should be considered to be garbage and
should never be used again anyhow.

Of course if the reason for returning oldstate is really so that the rng can
continue from where it left off then an extra internal state-machine state
should be considered so that neither initstate() or setstate() need to write to
the prior state structure.

I don't know - this seems really simple to me.  Why does it take lines of
detailed explanation just to state what should be obvious to an experienced and
very learned programmer like yourself. Or is defensive and safe programming too
much trouble?



Comment 5 Ulrich Drepper 2008-06-25 14:09:33 UTC
Feel free to contribute to the documentation if you think it is not obvious
enough for you.  The code does exactly what it is intended to do.  If in your
original code you wouldn't have blatantly ignored the return value of
initstate() it would have been easy to see that something has to be done with
this value.


Comment 6 JW 2008-06-25 14:42:35 UTC
You totally miss the point.

If you do:

     oldstate = setstate(newstate);
     ...
     setstate(oldstate)

then setstate(old) will actually write to newstate!

It isn't easy to see what MUST be done with oldstate if the documentation says
absolutely nothing about it.

Maybe you should call in an expert because you seem to be quite out of your
depth here.





Comment 7 JW 2008-06-25 14:50:03 UTC
Where is the documentation that states that initstate() will write data to the
saved pointer copy that is has.  Even though it is purportedly just setting a
new state structure.

Any given obvious omission in the documentation, why close this bug re poor
documentation and equally poor code?

What is wrong with you people? And why are you all employed by RedHat?  Where
are the non-RedHat people who might actually do something useful?  Or have you
chased all of them away too?




Comment 8 Ulrich Drepper 2008-06-25 14:52:26 UTC
I do not miss the point.  This is the expected behaviour.  You're pulling the
rug out from under the code and expect it to not fall over.  The return value of
initstate is the old state which you have to re-install before freeing the
memory allocated for the new state.  It's just ridiculous that you even try to
defend your mistake.

Comment 9 JW 2008-06-25 14:56:49 UTC
Where does the documentation say you have to do that?

It is ridiculous you say that the oldstate must be restored where they is no
documentation that says it must.

Where did you get this requirement from?



Comment 10 JW 2008-06-25 14:59:31 UTC
Also, the documentation says:

> The  state array state is used for random number generation
> until the next call to initstate() or  setstate().

Thats right!  It says the it is USED UNTIL THE NEXT CALL TO initstate().

But in reality it is used BEYOND the next call to in initstate().

So you are justing making things up to suit your own argument.



Comment 11 JW 2008-06-25 15:04:01 UTC
Please show me the documentation that says that oldstate must be restored.

And what if there is an error? What state is active then? Yes, it will still
write to some oldstate on the next call to initstate() after such an error.  But
which oldstate will that be?



Comment 12 JW 2008-06-25 15:05:24 UTC
And what is the purpose of initstate() writing to the previous state pointer? 
It doesn't make any sense.  It is bad and unnecessary code.


Comment 13 Jakub Jelinek 2008-06-25 15:08:24 UTC
Please stop reopening this bug.  When you don't understand why something is done
doesn't mean it doesn't make sense.

BTW, the reason why initstate_r and setstate_r writes to the previous state is
to make sure you can pass that state buffer to setstate{,_r} again.  Some part
of the state is stored in global variables (for the non-_r variants) or in the
buffer
passed to all _r variants.  initstate_r/setstate_r then flushes this global
vars/vars from the _r buffer into the previous arg state, switches to the new
state and reinitializes the global vars/vars in the _r buffer.


Comment 14 JW 2008-06-25 23:15:53 UTC
(In reply to comment #13)

> BTW, the reason why initstate_r and setstate_r writes to the previous state is
> to make sure you can pass that state buffer to setstate{,_r} again.

That can be achieved without writing to the old state structure.  I clearly
explained this previously. But in any case once a state structure is done with
there shouldn't be any guarantee that it can be used again, in fact it should be
totally reinitialized again if reused.

And please don't accuse me of not "understanding why something is done".  You
are the one who doesn't understand and who is just learning.  Look at the above
drivel you have just come up with after actually reading the source code for the
first time!  You would have done better to have just pasted the said code,
because it says everything that need be said.

I know what is being done, but I object to what is being done.  That is why I
say it doesn't make sense to do what it does.  It is unnecessary, bad and
dangerous and sloppy code.



Note You need to log in before you can comment on or make changes to this bug.