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

RESOLVED INVALID

Status

NSPR
NSPR
RESOLVED INVALID
9 years ago
9 years ago

People

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

Tracking

other
4.7.4
x86
Windows XP

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

9 years ago
Created attachment 348668 [details] [diff] [review]
Proposed patch

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)

Comment 1

9 years ago
> Aleksey, is this patch what you had in mind?

Yes. And the patch looks good!
(Assignee)

Updated

9 years ago
Attachment #348668 - Attachment is obsolete: true
Attachment #348668 - Flags: review?(aleksey)
(Assignee)

Comment 2

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

Comment 3

9 years ago
Created attachment 348870 [details] [diff] [review]
Add a comment

Aleksey, what do you think of this comment?
Attachment #348870 - Flags: review?(aleksey)

Comment 4

9 years ago
(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 :)

Comment 5

9 years ago
btw, patch looks good
(Assignee)

Comment 6

9 years ago
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
done
Attachment #348870 - Flags: review?(aleksey)
(Assignee)

Updated

9 years ago
Status: NEW → RESOLVED
Last Resolved: 9 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.