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

Status: RESOLVED INVALID

(NSPR :: NSPR, defect)

Description

[Wan-Teh Chang]

Attachment: 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?

Comment 1

[Aleksey Sanin]

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

Yes. And the patch looks good!

Comment 2

[Wan-Teh Chang]

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.

Comment 3

[Wan-Teh Chang]

Attachment: Add a comment

Aleksey, what do you think of this comment?

Comment 4

[Aleksey Sanin]

(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

[Aleksey Sanin]

btw, patch looks good

Comment 6

[Wan-Teh Chang]

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