-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Update random seed logic #262
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
Conversation
Thanks for the change. The This PR could however add a comment pointing that out for all the future readers of the code (myself included). For reference, the seed is set here: Line 93 in 7ffd74d
|
Actually I found this issue when running with "calibration-enable" during "several times" and it always gave me the error message with the same random string.... :-) Current code-base gives: Adding the seed as in the PR. |
Good catch! You are right. The autocalibration happens before Line 166 in 7ffd74d
main() . I think the initialization in job.Start() should still stay, as the job runner can be used from external packages as well.
|
I did some profiling on the amount of seed updates done when using this code from the PR. The code only calls the "seed update inside the RandomString function" 4 times per execution of the ffuf command. I think this current PR code will not harm performance and is cleaner to understand when the rand.Seed + rand.Intn are inside the same function block. :) |
I agree that with the current functionality it's not big of an issue. However I think that seeding the random every time a random value is needed is kind of an antipattern, and would have to be changed around whenever the function gets more use. Because of this I think it'd be better to try and find the proper place to do the random seeding. |
How about updating the rand.Seed after https://github.com/ffuf/ffuf/blob/master/pkg/ffuf/job.go#L333 ? |
rand.Seed updated just before usage
revert
Is this good to go? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a changelog entry to denote that this has been fixed? Otherwise, it's good to go!
CHANGELOG.md updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's good to go, thanks!
In GO rand.Intn is not really random at all unless you change the seed value before calling it.