Skip to content
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

fix: no-op math.randomseed() once called by Kong #1558

Merged
merged 1 commit into from
Aug 29, 2016

Conversation

thibaultcha
Copy link
Member

@thibaultcha thibaultcha commented Aug 26, 2016

Summary

This replaces #1554, since the improved seeding technique was not an
appropriate fix. To this point, I do not think it is worth investing in such a
complex seeding technique as the root of the issue really was the
continuous use of math.randomseed() by one of Lapis' utilities.

Changes

  • Disable (or rather, no-op) math.randomseed() to prevent
    third-party modules from overriding our correct seed (many modules
    make a wrong usage of math.randomseed() by calling it multiple times
    or do not use unique seed for Nginx workers.
  • Bump lua-resty-jit-uuid version to include uuid.seed() changes
  • Shorten the name of some of our cached variables

Update:

fix: patch math.randomseed() to prevent invalid calls …
Patch math.random to prevent it from being called by another context
than init_worker, and especially to prevent it from being called
multiple times. It is being patched very early in Kong's runtime so
we're sure no other module has time to cache the original math.random
function.

  • implement a globalpatches.lua module for modifying _G.
  • seed in init_worker with math.randomseed

ngx.log(ngx.DEBUG, "seeding random number generator for worker ",
ngx.worker.id(), " with: ", seed)
-- luacheck: globals math
math.randomseed = function()end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this too late? it replaces it when seeding, so any other module might have cached the original function and still call it.

So store the original in an upvalue, and replace upon module loading? and then even make sure it is loaded first...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw maybe cleaner to create a new module globalpatches or something similar. And just replace math.randomseed with the one from lua-resty-jit-uuid.

Or even update lua-resty-jit-uuid to make its randomseed signature like randomseed(seed, cmd) where cmd could be either string 'patch' (patches the global randomseed), or 'force' (forces a new randomseed anyway, to be called from worker_init).

@thibaultcha
Copy link
Member Author

Patch updated

@thibaultcha thibaultcha added this to the 0.9.1 milestone Aug 27, 2016
elseif not seed then
seed = ngx.time() * ngx.worker.pid()
ngx.log(ngx.DEBUG, "seeding random number generator for worker ",
ngx.worker.id(), " with: ", seed)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a typo but will also include pid

@Tieske
Copy link
Member

Tieske commented Aug 27, 2016

I like it in a new module for global patches, stands out cleanly.

lgtm!

@thibaultcha thibaultcha added pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release) and removed pr/status/needs review labels Aug 29, 2016
@thibaultcha thibaultcha force-pushed the fix/noop-randomseed branch 2 times, most recently from f1ae43f to ab69a62 Compare August 29, 2016 18:39
Patch `math.random` to prevent it from being called by another context
than init_worker, and especially to prevent it from being called
multiple times. It is being patched very early in Kong's runtime so
we're sure no other module has time to cache the original `math.random`
function.

* implement a `globalpatches.lua` module for modifying `_G`.
* seed in init_worker with `math.randomseed`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants