455974 - ssl_Poll seems to map lower_out_flags incorrectly.

Status: NEW

(NSS :: Libraries, defect, P5)

Description

[Wan-Teh Chang]

ssl_Poll is the poll method of the SSL layer's PRFileDesc.

After it calls the lower layer's poll method, it needs to
map the lower_out_flags and lower_new_flags returned by the
lower layer's poll method.  But this mapping (the code inside
the inner if statement shown below) seems incorrect:

    if (new_flags && (fd->lower->methods->poll != NULL)) {
        PRInt16    lower_out_flags = 0;
        PRInt16    lower_new_flags;
        lower_new_flags = fd->lower->methods->poll(fd->lower, new_flags,
                                                   &lower_out_flags);
        if ((lower_new_flags & lower_out_flags) && (how_flags != new_flags)) {
            PRInt16 out_flags = lower_out_flags & ~PR_POLL_RW;
            if (lower_out_flags & PR_POLL_READ)
                out_flags |= PR_POLL_WRITE;
            if (lower_out_flags & PR_POLL_WRITE)
                out_flags |= PR_POLL_READ;
            *p_out_flags = out_flags;
            new_flags = how_flags;
        } else {
            *p_out_flags = lower_out_flags;
            new_flags    = lower_new_flags;
        }
    }

Unfortunately, the lower layer is usually NSPR's TCP socket,
and its poll method simply does:

static PRInt16 pt_Poll(PRFileDesc *fd, PRInt16 in_flags, PRInt16 *out_flags)
{
    *out_flags = 0;
    return in_flags;
}  /* pt_Poll */

With such a poll method in the lower layer, the entire code snippet
of ssl_Poll that I showed above is a no-op.  This is why I can't
confirm this bug by testing with NSPR's TCP socket layer, and why
this bug is just a minor bug.

What seems wrong is that the mapping code blindly maps PR_POLL_READ
to PR_POLL_WRITE and vice versa in the out_flags.  If how_flags is
PR_POLL_READ, and we take this code path:

    } else if ((ss->lastWriteBlocked) && (how_flags & PR_POLL_READ) &&
               (ss->pendingBuf.len != 0)) { /* write data waiting to be sent */
        new_flags |=  PR_POLL_WRITE;   /* also select on write. */
    }

and if lower_out_flags is PR_POLL_READ, we will set out_flags to
PR_POLL_WRITE, which is wrong.

Comment 1

[Wan-Teh Chang]

Attachment: Proposed patch

I think something along this line should work.

This patch uses two boolean variables to designate two
conditions under which we need to map the out flags.

Unfortunately we can't test this patch with NSPR's
TCP socket.  Nelson, could you find out if the poll method
implementation of the Messaging Server's PRFileDesc
wrapper of their I/O library ever sets *out_flags to
a nonzero value?

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, 
The reversal of the read and write flags in that code was deliberate. 
As I recall, there are circumstances where we want to write, but must 
wait for an SSL record to be received first, and vice versa.  The code
you saw was intended to deal with those cases.  Unfortunately for me,
it's been a long time since I worked on that code, and I will have to 
study it again to refresh my memory about this.

I will send an email asking about the messaging server's implementation.

Comment 3

[Wan-Teh Chang]

Yes, I believe the reversal of the read and write flags is
correct in all cases except the case I mentioned in comment 0,
if we make the additional assumption that the caller isn't
using the SSL socket in full duplex mode (i.e., the caller is
polling for either read or write but not both) during SSL
handshake, which is a reasonable assumption.

Re: the messaging server's implementation: please find out
if their poll method ever sets *out_flags to a non-zero value.
If their poll method always sets *out_flags to 0, then the
out flags mapping code in ssl_Poll is a no-op.

My goal for this bug is to ensure that ssl_Poll can serve as
a model for poll method implementations.  Even though the code
in question is probably never executed in practice, I want to
make sure it at least looks correct.

Comment 4

[Wan-Teh Chang]

Edited the bug's summary.  ssl_Poll doesn't map lower_new_flags.

Comment 5

[Chris Newman]

I looked at Messaging server's three PRIO layers :-(.

ASock_PRPoll => sets *out_flags to zero.
(async I/O library)

prot/ssl_pr_poll => returns -1, sets PR_NOT_IMPLEMENTED_ERROR
(alternate protected I/O library)

asockprio.c/noop_poll => sets *out_flags to zero.
(noop I/O layer used to save SSL configurations -- the one I wrote)

Note that Messaging Server never calls PR_Poll, the async library uses OS poll directly.  The code relies on the lower ASock I/O layer (below the SSL layer) setting a flag when a read/write is EWOULDBLOCK/EAGAIN so the necessary direction is known for the SSL/reversal issue.

Comment 6

[Wan-Teh Chang]

Thanks, Nelson.  As I noted in comment 3, this means the out flags
mapping code in ssl_Poll is not being exercised by any code we know
of.

If the Messaging Server doesn't call PR_Poll or PRFileDesc's poll
method directly, these three poll methods (ASock_PRPoll,
prot/ssl_pr_poll, and asockprio.c/noop_poll) are really just
stubs that are never invoked.

Note that it doesn't make sense for a poll method to return -1
because the return value is a bitwise OR of bit flags.  No
return value indicates a failure, but I guess 0 is probably
more appropriate than -1 for the not-implemented stub
prot/ssl_pr_poll.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.