libc arc4_stir: use only kern.arandom sysctl
ClosedPublic
Actions

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

Details

Reviewers

jmg
markm
cperciva
ed
jonathan

Commits

rS306708: abort in srandomdev if kern.arandom sysctl fails
rS306636: libc arc4_stir: use only kern.arandom sysctl

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 - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

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.

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.

emaste added subscribers: oshogbo, allanjude, delphij, glebius.Oct 3 2016, 12:06 AM

emaste mentioned this in D1345: Capsicum support for jot(1).Oct 3 2016, 12:05 PM

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) ?

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).