Closed Bug 927230 Opened 11 years ago Closed 11 years ago

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

Categories

(NSS :: Libraries, defect, P2)

All
Linux
defect

Tracking

(Not tracked)

RESOLVED FIXED
3.15.4

People

(Reporter: wtc, Assigned: wtc)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #817590 - Flags: review?(ryan.sleevi)
Attached patch Patch v2 (obsolete) — Splinter Review
Also remove the unnecessary const casts for the file name argument to fopen().
Attachment #817590 - Attachment is obsolete: true
Attachment #817590 - Flags: review?(ryan.sleevi)
Attachment #817592 - Flags: review?(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.
Attachment #817592 - Flags: review?(ryan.sleevi) → review+
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.
Attached patch Patch v3Splinter Review
Do unbuffered I/O for all files.

Is the new comment accurate? Thanks.

Patch checked in: https://hg.mozilla.org/projects/nss/rev/beafc614009e
Attachment #817592 - Attachment is obsolete: true
Attachment #822066 - Flags: review?(ryan.sleevi)
Attachment #822066 - Flags: checked-in+
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Attachment #822066 - Flags: review?(ryan.sleevi) → review+
changing target milestone to 3.15.4
Target Milestone: 3.15.3 → 3.15.4
This causes a reliable startup crash on Android 2.3: Bug 935831. Backing out this patch fixes the crash.
Blocks: 935831
No longer blocks: 935831
Depends on: 935831
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Marked the bug fixed now that bug 935831 is fixed.
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: