927230 - Don't read from /dev/urandom using fully buffered mode I/O

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Wan-Teh Chang]

Attachment: Patch

Right now lib/freebl/unix_rand.c reads from /dev/urandom using
fopen() and fread(). By default the I/O is done in fully buffered
mode. On Linux, this seems to cause NSS to read 4096 bytes from
/dev/urandom even though NSS requests only 110 or 1024 bytes from
fread().

Ideally we should use open() and read() to read from /dev/urandom.
Unfortunately because Chromium's sandbox code doesn't intercept
the open() function for /dev/urandom, we need to continue to use
fopen() to open /dev/urandom. The attached patch simply calls
setvbuf() to use unbuffered I/O.

Comment 1

[Wan-Teh Chang]

Attachment: Patch v2

Also remove the unnecessary const casts for the file name argument to fopen().

Comment 2

[Ryan Sleevi]

Comment on attachment 817592 [details] [diff] [review]
Patch v2

Review of attachment 817592 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, but one question here

::: lib/freebl/unix_rand.c
@@ +976,4 @@
>      if (file != NULL) {
> +	/* Set buffering mode to unbuffered I/O to avoid reading more bytes
> +	 * than we need from /dev/urandom. */
> +	if (strcmp(fileName, "/dev/urandom") == 0) {

Why limit this to /dev/urandom? It seems we should do this for all the fileName, given that we repeat in a loop.

Comment 3

[Wan-Teh Chang]

Comment on attachment 817592 [details] [diff] [review]
Patch v2

Review of attachment 817592 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/freebl/unix_rand.c
@@ +976,4 @@
>      if (file != NULL) {
> +	/* Set buffering mode to unbuffered I/O to avoid reading more bytes
> +	 * than we need from /dev/urandom. */
> +	if (strcmp(fileName, "/dev/urandom") == 0) {

You are right. |buffer| has the size BUFSIZ, which is the size of a buffer to be used with setbuf(). So doing buffered I/O has no performance advantage.

Comment 4

[Wan-Teh Chang]

Attachment: Patch v3

Do unbuffered I/O for all files.

Is the new comment accurate? Thanks.

Patch checked in: https://hg.mozilla.org/projects/nss/rev/beafc614009e

Comment 5

[Kai Engert [:KaiE:]]

changing target milestone to 3.15.4

Comment 6

[Richard Newman [:rnewman]]

This causes a reliable startup crash on Android 2.3: Bug 935831. Backing out this patch fixes the crash.

Comment 7

[Wan-Teh Chang]

Marked the bug fixed now that bug 935831 is fixed.