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

RESOLVED FIXED in 3.15.4

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Wan-Teh Chang, Assigned: Wan-Teh Chang)

Tracking

trunk
3.15.4
All
Linux

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

2.05 KB, patch
Ryan Sleevi
: review+
Wan-Teh Chang
: checked-in+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Created attachment 817590 [details] [diff] [review]
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.
Attachment #817590 - Flags: review?(ryan.sleevi)
(Assignee)

Comment 1

5 years ago
Created attachment 817592 [details] [diff] [review]
Patch v2

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 2

5 years ago
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+
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Comment 4

5 years ago
Created attachment 822066 [details] [diff] [review]
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
Attachment #817592 - Attachment is obsolete: true
Attachment #822066 - Flags: review?(ryan.sleevi)
Attachment #822066 - Flags: checked-in+
(Assignee)

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Attachment #822066 - Flags: review?(ryan.sleevi) → review+

Comment 5

5 years ago
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
(Assignee)

Updated

5 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 7

5 years ago
Marked the bug fixed now that bug 935831 is fixed.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.