Last Comment Bug 357015 - On Windows, ReadSystemFiles reads 21 files as opposed to 10 files in C:\WINDOWS\system32.
: On Windows, ReadSystemFiles reads 21 files as opposed to 10 files in C:\WINDO...
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.11.3
: x86 Windows XP
: -- trivial (vote)
: 3.12
Assigned To: Wan-Teh Chang
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-10-17 13:41 PDT by Wan-Teh Chang
Modified: 2006-10-18 16:54 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch that illustrates the off-by-11 problem (1.16 KB, patch)
2006-10-17 14:03 PDT, Wan-Teh Chang
no flags Details | Diff | Review
Patch that illustrates the off-by-11 problem, v2 (1.27 KB, patch)
2006-10-17 14:11 PDT, Wan-Teh Chang
no flags Details | Diff | Review
Add a comment to describe what the code actually does (887 bytes, patch)
2006-10-18 16:06 PDT, Wan-Teh Chang
no flags Details | Diff | Review
Proposed patch (1.51 KB, patch)
2006-10-18 16:23 PDT, Wan-Teh Chang
no flags Details | Diff | Review
Proposed patch v2 (862 bytes, patch)
2006-10-18 16:53 PDT, Wan-Teh Chang
no flags Details | Diff | Review

Description Wan-Teh Chang 2006-10-17 13:41:55 PDT
On Windows (lib/freebl/win_rand.c), ReadSystemFiles says
"now read 10 files", but it actually reads 21 files in
the system directory (C:\WINDOWS\system32).
Comment 1 Wan-Teh Chang 2006-10-17 14:03:41 PDT
Created attachment 242555 [details] [diff] [review]
Patch that illustrates the off-by-11 problem

There are 2481 files in my C:\WINDOWS\system32 directory.

ReadSystemFiles intends to read one out of every 248 (= 2481/10)
files.  However, it has two mistakes.
1. It initializes filesToRead to 10, so it also reads the very
first 10 files in the directory listing.  (Note that we skip
files that we don't have the permission to read, so it's really
the first 10 readable files.)
2. It has an off-by-one error when it reads one file out of
every 248 files.

The following debug output shows the two mistakes.  The 2481 files
are numbered from 0 to 2480.  We show the files that are read and
the total number of files read.

Before this patch is applied:
2
11
12
16
17
18
19
20
21
22
23
248
496
744
992
1240
1488
1736
1984
2232
2480
*********** TotalRead  = 21 ********

After this patch is applied:
247
495
743
991
1239
1487
1735
1984
2231
2479
*********** TotalRead  = 10 ********
Comment 2 Wan-Teh Chang 2006-10-17 14:11:50 PDT
Created attachment 242560 [details] [diff] [review]
Patch that illustrates the off-by-11 problem, v2

This patch is better.
Comment 3 Nelson Bolyard (seldom reads bugmail) 2006-10-17 16:46:43 PDT
Wan-Teh, yes, it's true, the code reads the first 10 readable files, then 
10 files spread throughout the directory.  Is that a bug?
Perhaps the only "fix" needed is in the comment.
Comment 4 Wan-Teh Chang 2006-10-17 17:16:38 PDT
That's why the severity of this bug is "trivial" (cosmetic problem
like misspelled words or misaligned text) and I said my patch is
mainly intended to point out where the off-by-21 error comes from.

I am not picking nits in our source code.  I had to document how
we derive the seed for our deterministic RNG.  So I studied the
source code and traced it in the debuggers on several platforms.
The documentation is the "Security of key generation method"
subsection under 
http://wiki.mozilla.org/VE_07KeyMgmt#Key_Generation

I found bug 355297 (including the bug fixed by attachment 242022 [details] [diff] [review]),
bug 356884, bug 356886, bug 356595, and this bug in the code review
and debuggers. I documented our implementation faithfully, so
any oddity of our implementation, such as our passing the pseudo
handle returned by the GetCurrentProcess function to RNG_RandomUpdate
and our reading "at least 10 files" in C:\WINDOWS\system32, shows up
in the documentation.
Comment 5 Wan-Teh Chang 2006-10-18 16:06:35 PDT
Created attachment 242707 [details] [diff] [review]
Add a comment to describe what the code actually does

I added the comment that Nelson suggested on the trunk (NSS 3.12) only.

Checking in win_rand.c;
/cvsroot/mozilla/security/nss/lib/freebl/win_rand.c,v  <--  win_rand.c
new revision: 1.13; previous revision: 1.12
done
Comment 6 Wan-Teh Chang 2006-10-18 16:23:13 PDT
Created attachment 242709 [details] [diff] [review]
Proposed patch

This patch allows the code to read n files spread throughout
the system directory.  In this patch I set n=20 so that
the code reads as many files as it did before.  You can change
n back to the original 10 or any number you like.  I won't waste
anyone's time for a code review, so I'll mark the bug WONTFIX.
Comment 7 Nelson Bolyard (seldom reads bugmail) 2006-10-18 16:39:06 PDT
Wan-Teh, I disagree with the characterization that the code's behavior is 
due to a programming error.  It's doing what I intended it to do.
I agree that the comment was wrong.  Is that a programming error?
Comment 8 Wan-Teh Chang 2006-10-18 16:53:17 PDT
Created attachment 242711 [details] [diff] [review]
Proposed patch v2

I fixed the comment.  I didn't know that's the intention.
Sorry.  I'll let you decide if "10 or 11 files spread
throughout the system directory" is an off-by-one error.
I don't want to be a pest.

You can make that "10 files spread throughout the system
directory" by taking just the change to the ReadFiles
function from attachment 242709 [details] [diff] [review].
Comment 9 Wan-Teh Chang 2006-10-18 16:54:26 PDT
Comment fix checked in on NSS trunk (3.12).

Checking in win_rand.c;
/cvsroot/mozilla/security/nss/lib/freebl/win_rand.c,v  <--  win_rand.c
new revision: 1.14; previous revision: 1.13
done

Note You need to log in before you can comment on or make changes to this bug.