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)
Tracking
(Not tracked)
RESOLVED
FIXED
3.15.4
People
(Reporter: wtc, Assigned: wtc)
References
Details
Attachments
(1 file, 2 obsolete files)
2.05 KB,
patch
|
ryan.sleevi
:
review+
wtc
:
checked-in+
|
Details | Diff | 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)
Assignee | ||
Comment 1•11 years ago
|
||
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•11 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•11 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•11 years ago
|
||
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•11 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Attachment #822066 -
Flags: review?(ryan.sleevi) → review+
Comment 6•11 years ago
|
||
This causes a reliable startup crash on Android 2.3: Bug 935831. Backing out this patch fixes the crash.
Blocks: 935831
Updated•11 years ago
|
Assignee | ||
Updated•11 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 7•11 years ago
|
||
Marked the bug fixed now that bug 935831 is fixed.
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•