Bug 199480 - lang/lua52, lang/lua53: Use 'arc4random()' instead of 'time()' for string hash seed
Summary: lang/lua52, lang/lua53: Use 'arc4random()' instead of 'time()' for string has...
Status: Open
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Chris Rees
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-16 12:11 UTC by Vitaly Magerya
Modified: 2019-09-10 03:52 UTC (History)
7 users (show)

See Also:
bugzilla: maintainer-feedback? (lua)


Attachments
lua-makeseed.diff (1.45 KB, patch)
2015-04-16 12:11 UTC, Vitaly Magerya
no flags Details | Diff
lua-arc4random.diff (1.71 KB, patch)
2015-12-30 20:08 UTC, Vitaly Magerya
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vitaly Magerya 2015-04-16 12:11:13 UTC
Created attachment 155642 [details]
lua-makeseed.diff

When hashing strings into tables, Lua 5.2 uses a semi-random
hash seed, which is obtained from a mixture of 'time()' and a
few global addresses (in hope that ASLR would randomize those).
See the 'makeseed' function in 'src/lstate.c' for how it's done.

Since we don't have ASLR, the resulting seed depends only on
time(), which means it can only change once a second, instead
of at every execution.

I think this randomization was meant as a way to prevent DoS
attacks based on hash table collisions (I'm not sure if it's
effective, since the random seed is only computed once at startup).
In any case, the biggest effect this has for me is that performance
of various code parts depends on the ordering of table keys (and
thus, on the seed), but you only get one seed per second, so
running microbenchmarks suddenly becomes a problem.

Now, 'src/lstate.c' provides a way for us to supply a better
seed: we just need to redefine 'luai_makeseed' (by default it's
just a 'time()' call).

The attached patch changes 'luai_makeseed' into an 'arc4random()'
call for both lang/lua52 and lang/lua53 (lua51 doesn't seem to
have an equivalent randomization).

Note that defining __BSD_VISIBLE is only really needed for lua53,
since it defines _XOPEN_SOURCE somewhere above, and without
__BSD_VISIBLE 'arc4random' prototype is not visible with just a
'stdlib.h' include.
Comment 1 Baptiste Daroussin freebsd_committer 2015-04-16 12:42:58 UTC
this would have to be validated by upstream first imho
Comment 2 Vitaly Magerya 2015-04-16 13:21:19 UTC
The upstream accepts no patches, certainly not the unportable
ones... Would you like me to to go lua-l and ask if it's OK to
change 'time()' into 'arc4random()' on FreeBSD?
Comment 3 Carlo Strub freebsd_committer 2015-09-15 17:15:47 UTC
Maybe that's the right thing to do? Let's see what feedback you get.
Comment 4 Martin Wilke freebsd_committer 2015-12-24 20:51:07 UTC
Hi,

Any update on this?
Comment 5 Vitaly Magerya 2015-12-30 20:08:10 UTC
Created attachment 164875 [details]
lua-arc4random.diff

The update is that the upstream doesn't mind [1].

Also, based on their suggestions, here's a new (less intrusive)
patch.

Note that the new patch fixes what seems to be a mistake in
lang/lua53/files/patch-src__Makefile: currently that file changes
"CFLAGS= ..." into "CFLAGS?= ...", which makes it so that the
"..." part is ignored, and the "MYCFLAGS" variable defined from
lang/lua53/Makefile is never used. Note that the equivalent patch
for lua52 (lang/lua53/files/patch-src__Makefile) does not add
the "?", which is the correct way to do things.

[1] http://article.gmane.org/gmane.comp.lang.lua.general/120118
Comment 6 Mark Felder freebsd_committer 2016-03-18 12:49:10 UTC
Since upstream seems to have no problem with this, are there any other outstanding objections preventing this from being committed?
Comment 7 Chris Rees freebsd_committer 2019-09-09 21:22:57 UTC
Perhaps the new maintainer should be aware of this!

Russell, what do you think?
Comment 8 Chris Rees freebsd_committer 2019-09-09 21:23:12 UTC
(I'll get this in)
Comment 9 Russell Haley 2019-09-10 03:52:33 UTC
Hi,

Would this be a ports option or are you proposing a straight patch? Will you be posting something on phabricator, or shall I take a crack at it?