libc arc4_stir: use only kern.arandom sysctl

Authored by emaste on Sep 29 2016, 5:39 PM.



The sysctl cannot fail. If it does fail on some FreeBSD derivative or after some future change, just abort() so that the problem will be found and fixed.

It's preferable to provide an arc4random() function that cannot fail and cannot return poor quality random data. While abort() is not normally suitable for a library, it makes sense here.

Diff Detail

rS FreeBSD src repository
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.
emaste updated this revision to Diff 20820.Sep 29 2016, 5:39 PM
emaste retitled this revision from to libc arc4_stir: use only kern.arandom sysctl.
emaste updated this object.
emaste edited the test plan for this revision. (Show Details)
emaste added reviewers: markm, cperciva, jmg.
markm accepted this revision.Oct 2 2016, 3:32 PM
markm edited edge metadata.

I really like the direction. One minor nit which is more of a question than an objection.

141 ↗(On Diff #20820)

Simpler. Good.

148 ↗(On Diff #20820)

Shouldn't this be a KASSERT()?

149 ↗(On Diff #20820)

Nice to see this kind of yucky code go.

150 ↗(On Diff #20820)


This revision is now accepted and ready to land.Oct 2 2016, 3:32 PM
emaste added inline comments.Oct 2 2016, 6:27 PM
148 ↗(On Diff #20820)

This is the libc userland side

markm added inline comments.Oct 2 2016, 7:42 PM
148 ↗(On Diff #20820)

I meant ASSERT() :-)

emaste added inline comments.Oct 2 2016, 7:52 PM
148 ↗(On Diff #20820)

Ah. I'm worried about the case where it's compiled without assertions enabled though, leading back to a possible silent failure.

ed accepted this revision.Oct 3 2016, 12:55 PM
ed added a reviewer: ed.
ed added a subscriber: ed.
ed added inline comments.
148 ↗(On Diff #20820)

Letting this use abort() is good. Because this is crossing the user-kernel boundary, it's not an internal invariant of the C library that's failing. Only in those cases we should use assert().

oshogbo added inline comments.Oct 3 2016, 12:57 PM
148 ↗(On Diff #20820)

Can't we just do
ret = arc4_sysctl(...)
assert(ret == KEYSIZE) ?

jonathan accepted this revision.Oct 3 2016, 1:05 PM
jonathan added a reviewer: jonathan.
jonathan added a subscriber: jonathan.

Very nice.

emaste added inline comments.Oct 3 2016, 1:08 PM
148 ↗(On Diff #20820)

As @ed said, assert() is for internal invariants of the code in question, and it may be compiled out (if built with -NDEBUG). A failure here would be due to a change external to libc (e.g., someone changes the kernel).

This revision was automatically updated to reflect the committed changes.