Bug 362161 - KRandom::random seeds qrand, but returns value from rand
Summary: KRandom::random seeds qrand, but returns value from rand
Status: RESOLVED FIXED
Alias: None
Product: frameworks-kcoreaddons
Classification: Frameworks and Libraries
Component: general (show other bugs)
Version: 5.0.0
Platform: Mageia RPMs Linux
: NOR normal
Target Milestone: ---
Assignee: Michael Pyne
URL:
Keywords:
: 361642 362093 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-04-23 19:40 UTC by Arne Spiegelhauer
Modified: 2016-05-08 23:42 UTC (History)
7 users (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments
patch proposal for krandom test (1.95 KB, patch)
2016-05-08 20:48 UTC, Arne Spiegelhauer
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Arne Spiegelhauer 2016-04-23 19:40:23 UTC
KRandom::random was recently changed to use qrand and qsrand, but the call to rand in the return statement was apparently missed


Reproducible: Always

Steps to Reproduce:
1. start e.g. kshisen
2. quit kshisen
3. start kshisen again

Actual Results:  
1. and 3. brings up the same inital board

Expected Results:  
initial board should be different on each start
Comment 1 Frederik Schwarzer 2016-04-23 20:07:03 UTC
Adding Aleix since he committed the change.
@Aleix, can you please review this issue?
Comment 2 Frederik Schwarzer 2016-04-23 20:08:58 UTC
*** Bug 362093 has been marked as a duplicate of this bug. ***
Comment 3 Aleix Pol 2016-04-25 23:59:38 UTC
Check that /dev/urandom can be opened? It seems to work here.
Comment 4 Arne Spiegelhauer 2016-04-26 12:06:13 UTC
The problem was introduced in version 5.21.0 (which cannot be selected from the version drop-down menu above)

The function is:

int KRandom::random()
{
    static bool init = false;
    if (!init) {
        unsigned int seed;
        init = true;
        QFile urandom(QStringLiteral("/dev/urandom"));
        bool opened = urandom.open(QIODevice::ReadOnly | QIODevice::Unbuffered);
        if (!opened || urandom.read(reinterpret_cast<char *>(&seed), sizeof(seed)) != sizeof(seed)) {
            // No /dev/urandom... try something else.
            qsrand(getpid());
            seed = qrand() + time(0);
        }
        qsrand(seed);
    }
    return rand();
}

On first call after program load, the qrand() random number generator is seeded by the call of qsrand(seed).
However, the value returned by KRandom::random() is obtained by calling the rand() random  number generator, which has not been seeded, and thus returns the same sequence of numbers after each re-start of the calling program.
Comment 5 Daniel Pfenniger 2016-04-26 13:14:00 UTC
Random numbers may be of crucial importance for security and cryptography, so such functions should be always automatically passed through standard randomness tests before each new KDE release.
Comment 6 Albert Astals Cid 2016-04-26 21:42:08 UTC
Git commit 78212436643af95779facd9593c82fb149c2213d by Albert Astals Cid.
Committed on 26/04/2016 at 21:41.
Pushed by aacid into branch 'master'.

Missing rand() -> qrand

Fixes regression introduced in 9ae6d765b37135bbfe3a8b936e5a88b8a435e424

Reviewed by Aleix

M  +1    -1    src/lib/randomness/krandom.cpp

http://commits.kde.org/kcoreaddons/78212436643af95779facd9593c82fb149c2213d
Comment 7 Albert Astals Cid 2016-04-26 21:42:49 UTC
Daniel, please provide a patch with such a test :)Daniel, please provide a patch with such a test :)
Comment 8 Michael Pyne 2016-04-28 01:10:45 UTC
Git commit 8f5cdadca7fad0265204d5f9ff16bc8ca1c02f72 by Michael Pyne.
Committed on 28/04/2016 at 01:08.
Pushed by mpyne into branch 'master'.

autotests: Add test case for KRandom{,Sequence}.

Very simple, but more tests than we had before. However this would not
have caught the bug that prompted this since it would require running
different processes consecutively to reproduce. Patches welcome! ;)
REVIEW:127778

M  +1    -0    autotests/CMakeLists.txt
A  +118  -0    autotests/krandomtest.cpp     [License: LGPL (v2)]

http://commits.kde.org/kcoreaddons/8f5cdadca7fad0265204d5f9ff16bc8ca1c02f72
Comment 9 Daniel Pfenniger 2016-04-28 11:55:09 UTC
(In reply to Albert Astals Cid from comment #7)
> Daniel, please provide a patch with such a test :)Daniel, please provide a
> patch with such a test :)

Alas, I am absolutely ignorant about KDE internals and C++. :)
Apparently the KDE dev team does standard tests, just what I was ignoring, and 
this bug will trigger an additional test, which is exactly what should be done.
Comment 10 Frederik Schwarzer 2016-05-02 12:50:31 UTC
*** Bug 361642 has been marked as a duplicate of this bug. ***
Comment 11 Arne Spiegelhauer 2016-05-08 20:48:24 UTC
Created attachment 98848 [details]
patch proposal for krandom test

(In reply to Michael Pyne from comment #8)
> 
> Very simple, but more tests than we had before. However this would not
> have caught the bug that prompted this since it would require running
> different processes consecutively to reproduce. Patches welcome! ;)

I have uploaded a test patch proposal. I have verified that the test fails with bug 362161 and passes with the fix implemented.
Please comment if you have the time
Comment 12 Michael Pyne 2016-05-08 23:42:20 UTC
Git commit 66c439719f60c5239975dc44178e37ce0bb1c08c by Michael Pyne, on behalf of Arne Spiegelhauer.
Committed on 08/05/2016 at 23:39.
Pushed by mpyne into branch 'master'.

krandom: Add testcase to catch bug 362161 (failure to auto-seed).

Kindly contributed by Arne Spiegelhauer, with some minor touch-ups by
myself. Thanks for the patch and for helping us to avoid reintroducing
the original bug later!

M  +67   -1    autotests/krandomtest.cpp

http://commits.kde.org/kcoreaddons/66c439719f60c5239975dc44178e37ce0bb1c08c