Fix invalid memory access in TRandom3#14135
Conversation
The TRandom3 generator was observed to fail
a very simple test on the SetSeed/GetSeed interface:
```
gRandom->SetSeed(11);
auto a = gRandom->GetSeed();
gRandom->SetSeed(12);
auto b = gRandom->GetSeed();
assert(a != b);
```
Indeed a `GetSeed()` following any `SetSeed(seed)` call
always returns the magic number 624. This is because
in the current implementation
`GetSeed() { return fMT[fCount624]; }`
we access memory location `fMT[624]` which does not
exist in fMT ... and so the value of fCount624 is returned,
which happens to be `624`.
This commit fixes this bug by imposing an index range
between 0 and 623.
|
Can one of the admins verify this patch? |
lmoneta
left a comment
lmoneta
left a comment
There was a problem hiding this comment.
Thank you for fixing this memory access problem.
However, for TRandom3, with a state of 624 words, it does not make any sense to return a single seed since there is no guarantee that the other 623 values will be the same.
Maybe is better to return a zero value.
Hi Lorenzo. No, I believe we should leave the current contract intact, which I am fixing. I am relying in my code on the fact that |
|
I see, but this can be dangerous if misused. What is the usage you are doing with the return value of |
ROOTs TRandom3::GetSeed() may return non-sensical values when we call GetSeed() directly after a GetSeed(). An intermediate call to TRandam3::Rndm() avoids this misbehaviour. See also root-project/root#14135, where this is fixed directly in ROOT.
|
I agree that the interface is somewhat dangerous but the docs clearly mention its limitations. However, dangerous or not, this is currently broken and should be fixed. |
ROOTs TRandom3::GetSeed() may return non-sensical values when we call GetSeed() directly after a GetSeed(). An intermediate call to TRandam3::Rndm() avoids this misbehaviour. See also root-project/root#14135, where this is fixed directly in ROOT.
ROOTs TRandom3::GetSeed() may return non-sensical values when we call GetSeed() directly after a SetSeed(...). An intermediate call to TRandam3::Rndm() avoids this misbehaviour. See also root-project/root#14135, where this is fixed directly in ROOT.
ROOTs TRandom3::GetSeed() may return non-sensical values when we call GetSeed() directly after a SetSeed(...). An intermediate call to TRandam3::Rndm() avoids this misbehaviour. See also root-project/root#14135, where this is fixed directly in ROOT.
math/mathcore/inc/TRandom3.h
Outdated
| @@ -35,7 +35,7 @@ class TRandom3 : public TRandom { | |||
| ~TRandom3() override; | |||
| /// return current element of the state used for generate the random number | |||
There was a problem hiding this comment.
I think we should specify clearly here what is return
Maybe something like:
Return one element of the generator state used to generate the random numbers.
Note that it is not the seed of the generator that was used in the SetSeed function and the full state (624 numbers)
is required to define the generator and have a reproducible output.
There was a problem hiding this comment.
Yes that's ok with me. You may just modify my PR accordingly.
ROOTs TRandom3::GetSeed() may return non-sensical values when we call GetSeed() directly after a SetSeed(...). An intermediate call to TRandam3::Rndm() avoids this misbehaviour. See also root-project/root#14135, where this is fixed directly in ROOT.
Document better what GetSeed returns for TRandom3
|
The PR looks good now. Thank you @sawenzel for this fix |
ROOTs TRandom3::GetSeed() may return non-sensical values when we call GetSeed() directly after a SetSeed(...). An intermediate call to TRandam3::Rndm() avoids this misbehaviour. See also root-project/root#14135, where this is fixed directly in ROOT.
ROOTs TRandom3::GetSeed() may return non-sensical values when we call GetSeed() directly after a SetSeed(...). An intermediate call to TRandam3::Rndm() avoids this misbehaviour. See also root-project/root#14135, where this is fixed directly in ROOT.
ROOTs TRandom3::GetSeed() may return non-sensical values when we call GetSeed() directly after a SetSeed(...). An intermediate call to TRandam3::Rndm() avoids this misbehaviour. See also root-project/root#14135, where this is fixed directly in ROOT.
4 participants
The TRandom3 generator was observed to fail
a very simple test on the SetSeed/GetSeed interface:
Indeed a
GetSeed()following anySetSeed(seed)call always returns the magic number 624. This is because in the current implementationGetSeed() { return fMT[fCount624]; }we access memory location
fMT[624]which does not exist in fMT ... and so the value of fCount624 is returned, which happens to be624.This commit fixes this bug by imposing an index range between 0 and 623.