Closed Bug 465435 Opened 12 years ago Closed 12 years ago

w32poll.c: _PR_MD_PR_POLL() should check against FD_SETSIZE before the FD_SET() calls


(NSPR :: NSPR, defect)

Windows XP
Not set


(Not tracked)



(Reporter: wtc, Assigned: wtc)



(1 file, 1 obsolete file)

Attached patch Proposed patch (obsolete) — Splinter Review
This bug is reported by Aleksey Sanin of Coral8, Inc.

In w32poll.c, the _PR_MD_PR_POLL() function is checking against FD_SETSIZE:

250     if ((nrd > FD_SETSIZE) || (nwt > FD_SETSIZE) || (nex > FD_SETSIZE)) {
251         PR_SetError(PR_INVALID_ARGUMENT_ERROR, 0);
252         return -1;
253     }

This check should be moved up into the for loop above it (and ">" replaced
with ">=") to protect the FD_SET() calls in the for loop.

Aleksey, is this patch what you had in mind?
Attachment #348668 - Flags: review?(aleksey)
> Aleksey, is this patch what you had in mind?

Yes. And the patch looks good!
Attachment #348668 - Attachment is obsolete: true
Attachment #348668 - Flags: review?(aleksey)
Comment on attachment 348668 [details] [diff] [review]
Proposed patch

It turns out the original code is correct.  FD_SET does the
check against FD_SETSIZE and does nothing if the fd array is
full.  (I just verified this with the winsock2.h and winsock.h
in Visual C++ 6.)  So we just need to do our check after the
for loop to detect if any of the fd arrays is full.

I'll add a comment so that we won't forget this again.
Attached patch Add a commentSplinter Review
Aleksey, what do you think of this comment?
Attachment #348870 - Flags: review?(aleksey)
(In reply to comment #3)
> Created an attachment (id=348870) [details]
> Add a comment
> Aleksey, what do you think of this comment?

Sorry, I missed it. I think that this relies on un-documented behavior of FD_SET() and friends that can easily change in the next Windows SDK release. Though it is probably unlikely. 

Anyway, a comment would be nice. I was puzzled :)
btw, patch looks good
Comment on attachment 348870 [details] [diff] [review]
Add a comment

I agree that this undocumented check in FD_SET is unlikely to change
in future versions of the Platform SDK.

I checked in this comment patch on the NSPR trunk (NSPR 4.7.4).

Checking in w32poll.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/w32poll.c,v  <--  w32poll.c
new revision: 3.13; previous revision: 3.12
Attachment #348870 - Flags: review?(aleksey)
Closed: 12 years ago
Resolution: --- → INVALID
Target Milestone: --- → 4.7.4
You need to log in before you can comment on or make changes to this bug.