121951 - PR_Poll does not work with layered file descriptors (e.g., SSL) on Mac

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Wan-Teh Chang]

The Mac implementation of PR_Poll does not work with
layered file descriptors.  (SSL file descriptors are
layered.)  This bug was not exposed before because
we have been doing blocking SSL connect.  With the
proposed patch in bug 106188, we will be doing
non-blocking SSL connect, and PR_Poll will be called
on the SSL file descriptor after a non-blocking
connect is initiated.  Bad things can happen because
of the interaction between PR_Poll and the 'poll'
method of an SSL FD.

Comment 1

[Simon Fraser [no longer active]]

Attachment: Changes to MD_Poll to fix the problem

These changes to MD_Poll for Mac hopefully make it work for layered poll
methods.
I broke CheckPollDescs() into two functions:
CheckPollDescMethods(), which

Comment 2

[Simon Fraser [no longer active]]

Gack, aborted comment.

These changes to MD_Poll for Mac hopefully make it work for layered poll methods.
I broke CheckPollDescs() into two functions:
* CheckPollDescMethods(), which checks the state of the poll descriptors via their
  poll methods (layering), and
* CheckPollDescEndpoints(), which checks the OT endpoints of the poll descriptors.

This allows us to disable interrupts and lock the asyncIOLock only around calls
to CheckPollDescEndpoints() which we make to determine whether to wait for IO.

There are two important aspects of the patch:
1. Arrays of read/write flags are passed between CheckPollDescMethods and 
   CheckPollDescEndpoints to enable the return flags of the poll method calls
   to affect the getting of read/write states of the endpoints (thus maintaining
   the behavior of the old code).

2. Interrupts are disabled and the asyncIOLock is locked around the second call 
   to CheckPollDescEndpoints(), the result of which we use to determine whether
   to wait for IO. Note that we don't have to lock around the first call; it's
   OK if we miss notifications, because the second call with catch them. What
   is important is to disable interrupts and lock around the call that is
   used to determine whether to wait. What we need to avoid here is waiting
   after a notification has already come in.

With this patch, non-blocking SSL connects seem to work fine, and FTP/mail work.
As a note, I found that FTP was most sensitive to race conditions in this code.

Comment 3

[Simon Fraser [no longer active]]

Attachment: Same patch with all the whitespace changes.

Comment 4

[Mike Pinkerton (not reading bugmail)]

i tried the latest patch and didn't see any problems with imap, secure imap,
https, news, or ftp on my dual800.

Comment 5

[Wan-Teh Chang]

Good job, Simon!

Please give me until next Monday to review your patch.
From a cursory review, the only issue I spotted is the
use of the two arrays:
    PRInt16     readFlagsArray[DESCRIPTOR_FLAGS_ARRAY_SIZE];
    PRInt16     writeFlagsArray[DESCRIPTOR_FLAGS_ARRAY_SIZE];
On Unix and Windows, I believe I avoid these two arrays
by using the out_flag fields of the PRPollDesc structures
as the temporary scratch arrays.  Please take a look at
mozilla/nsprpub/pr/src/pthreads/ptio.c, _pr_poll_with_poll(),
especially the section where I store _PR_POLL_READ_SYS_READ
etc. temporarily in pds[index].out_flags.

Comment 6

[Simon Fraser [no longer active]]

Using pd->out_flags as a scratch array scares me. _pr_poll_with_poll seems to be 
able to do this because it allocates a parallel array of pollfds. In addition, I 
use the values in pd->out_flags to count the ready descriptors a few times.

Comment 7

[Wan-Teh Chang]

Comment on attachment 66964 [details] [diff] [review]
Same patch with all the whitespace changes.

Simon,

I like this patch.  I would like to suggest some
improvement.

1. CheckPollDescs() was split into three functions,
but the new functions all have the same original
comment in front of the function prototypes.  Please
update the comments to describe what each new
function does.

2. The value of DESCRIPTOR_FLAGS_ARRAY_SIZE is 16.
Please verify that this is large enough for Mozilla
client to avoid the PR_Malloc() and PR_Free() calls.

3. _MD_poll() only needs to call CheckPollDescMethods()
once and the number of CheckPollDescEndpoint() calls
can be reduced from three to two.  Here is what
_MD_poll() might look like:

    PRBool needToWait = PR_FALSE;
    intn is;

    (void)CheckPollDescMethods(...);
    _PR_INTSOFF(is);
    PR_Lock(thread->md.asynchIOLock);
    (void)CheckPollDescEndPoints(...);
    ready = CountReadyPollDescs(...);
    if (ready == 0 && timeout != PR_INTERVAL_NO_WAIT)
    {
	PrepareForAsyncCompletion(thread, 0);
	SetDescPollThread(pds, npds, thread);
	needToWait = PR_TRUE;
    }
    PR_Unlock(thread->md.asynchIOLock);
    _PR_FAST_INTSON(is);

    if (needToWait)
    {
	WaitOnThisThread(thread, timeout);
	ready = CheckPollDescEndpoints(...);
	SetDescPollThread(pds, npds, NULL);
    }

Do you think this would work?

Comment 8

[Simon Fraser [no longer active]]

    if (needToWait)
    {
        WaitOnThisThread(thread, timeout);
        ready = CheckPollDescEndpoints(...);
        ...

This won't work. If the thread is woken via a pollable event, 'ready' won't 
account for that. This is why we need to call CheckPollDescMethods(...)
after waiting.

Comment 9

[Wan-Teh Chang]

Will this work?

    if (needToWait)
    {
        WaitOnThisThread(thread, timeout);
        (void)CheckPollDescEndMethods(...);
        (void)CheckPollDescEndpoints(...);
        ready = CountReadyPollDescs(pds, npds);
        ...

So how does a pollable event wake up the polling thread on
the Mac?  You seem to suggest that it does that by making
the poll method indicate the pollable event is ready.  Is
the pollable event implemented without using any underlying
file, pipe, or socket?

Comment 10

[Simon Fraser [no longer active]]

Attachment: macksockotpt.c patch with wtc's suggested logic

Comment 11

[Simon Fraser [no longer active]]

Attachment: macsockotpt.c file in its entirety, for ease of viewing

Comment 12

[Simon Fraser [no longer active]]

I've tested these changes on a dual CPU machine with OS X, with FTP, SSL, IMAP
and browser, and everything seems to work fine.

Comment 13

[Wan-Teh Chang]

Comment on attachment 69356 [details] [diff] [review]
macksockotpt.c patch with wtc's suggested logic

Simon,

This patch is good.  Please go ahead and check it into
the tip and client branch of NSPR.  Thanks.

Comment 14

[gordon]

r=gordon, if you want it :-)

Thanks Simon.

Comment 15

[Simon Fraser [no longer active]]

Patch checked into NSPRPUB_PRE_4_2_CLIENT_BRANCH and the NSPR tip.