libc arc4_stir: use only kern.arandom sysctl
ClosedPublic

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

Details

Summary

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

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
emaste retitled this revision from to libc arc4_stir: use only kern.arandom sysctl.Sep 29 2016, 5:39 PM
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

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

lib/libc/gen/arc4random.c
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)

Tidier!

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
lib/libc/gen/arc4random.c
148 ↗(On Diff #20820)

This is the libc userland side

markm added inline comments.Oct 2 2016, 7:42 PM
lib/libc/gen/arc4random.c
148 ↗(On Diff #20820)

I meant ASSERT() :-)

emaste added inline comments.Oct 2 2016, 7:52 PM
lib/libc/gen/arc4random.c
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.
lib/libc/gen/arc4random.c
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
lib/libc/gen/arc4random.c
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
lib/libc/gen/arc4random.c
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.