New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HPCC-18585 Replace uses of rand() with fastRand() #10540
Conversation
https://track.hpccsystems.com/browse/HPCC-18585 |
@richardkchapman - please review |
Looking at the implementation of getRandom(): unsigned getRandom() this may not be such a brilliant idea |
Perhaps for the cases where all we want is a fast pseudo-random number for collision-avoidance or randomized testing purposes that does not need to be cryptographically secure, we should define something in jlib (call it fastrand() or similar) that just calls rand() but leaves us with only one place to mark as a coverity exception ? |
Yes, probably. There may be existing places that call getRandom() which are more time critical, but I don't think the places changed in this PR are / would be impacted by the crit entry / leave. |
Apparently rand() isn't thread safe anyway, though not sure what implications of re-entry are. |
Introduced fastRand() with coverity suppression comment. @richardkchapman - please review. |
The one coverity was complaining about specifically (on this occasion) was in datest.cpp, but does not appear to have been addressed here? |
Coverity can warn of an unsafe usage of rand(), where we want a fast random call and know it's in a safe context explicitly use one. With an initial implementation that simply wraps rand() with a coverity comment to suppress the WEAK_CRYPTO Coverity warning. Signed-off-by: Jake Smith <jake.smith@lexisnexisrisk.com>
@richardkchapman - pushed wrong commit, re-pushed. |
Automated Smoketest: ✅
Install hpccsystems-platform-community_6.5.0-trunk0.el7.x86_64.rpm Unit tests result:
Regression test result:
HPCC Stop: OK |
Coverity can warn of an unsafe usage of rand(), where we want
a fast random call and know it's in a safe context explicitly use
one.
With an initial implementation that simply wraps rand() with a
coverity comment to suppress the WEAK_CRYPTO Coverity warning.
Signed-off-by: Jake Smith jake.smith@lexisnexisrisk.com
Type of change:
Checklist:
Testing:
Unit tests ran/passed.