155489 - A possible misuse of the out_flags argument of the poll method by PR_Poll

Status: RESOLVED INCOMPLETE

(NSPR :: NSPR, defect)

Description

[Wan-Teh Chang]

I am documenting NSPR's poll method.  While reviewing the implementation
of PR_Poll (the only NSPR function using the poll method), I found what
looks like a misinterpretation of the out_flags argument of the poll
method.  Unfortunately, since there is no documentation or even comments
in the source code about the poll method, the current implementation
of PR_Poll is effectively the specification of the poll method.  If this
is indeed a misinterpretation of the out_flags argument of the poll
method, I am afraid that we will have to live with it until the next NSPR
major release (5.0).

In the current implementation of PR_Poll, we have (code snippet from
_pr_poll_with_poll in ptio.c):

                if (pds[index].in_flags & PR_POLL_READ)
                {
                    in_flags_read = (pds[index].fd->methods->poll)(
                        pds[index].fd,
                        pds[index].in_flags & ~PR_POLL_WRITE,
                        &out_flags_read);
                }
                if (pds[index].in_flags & PR_POLL_WRITE)
                {
                    in_flags_write = (pds[index].fd->methods->poll)(
                        pds[index].fd,
                        pds[index].in_flags & ~PR_POLL_READ,
                        &out_flags_write);
                }
                if ((0 != (in_flags_read & out_flags_read))
                || (0 != (in_flags_write & out_flags_write)))
                {
                    /* this one is ready right now */
                    if (0 == ready)
                    {
                        /*
                         * We will return without calling the system
                         * poll function.  So zero the out_flags
                         * fields of all the poll descriptors before
                         * this one.
                         */
                        int i;
                        for (i = 0; i < index; i++)
                        {
                            pds[i].out_flags = 0;
                        }
                    }
                    ready += 1;
==>                 pds[index].out_flags = out_flags_read | out_flags_write;
                }

The second-to-last line (pds[index].out_flags = ...) assumes that
out_flags_read and out_flags_write (the values that the poll method
stores in the out_flags argument) are the poll events reflecting
the view of the caller.  I believe that is a misinterpretation of
the out_flags argument of the poll method.  I based that on the
MyPoll method in the nblayer.c test (the only sample code of the
poll method in NSPR) and the old implementation of PR_Poll.  It
seems that the original intent of the out_flags is that out_flags
may reflect the view of any I/O layer in the stack as long as the
return value of the poll method also reflect the view of the same
I/O layer.  This is so that the poll method of an I/O layer can
simply invoke the poll method of the lower layer without having to
map the out_flags and return value back to the poll events at the
caller's level.

I think that line of code should be replaced by:

                    pds[index].out_flags = 0;
                    if (0 != (in_flags_read & out_flags_read))
                        pds[index].out_flags |= PR_POLL_READ;
                    if (0 != (in_flags_write & out_flags_write))
                        pds[index].out_flags |= PR_POLL_WRITE;

With this change, the poll event mapping code in some poll methods
can be deleted.  For example, the following code at the end of NSS's
ssl_Poll method (in sslsock.c) maps the poll events for the lower
layer to the poll events for the caller:

    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;
        }
    }

With the proposed change to PR_Poll, the above code in ssl_Poll
can be simplified to:

    if (new_flags && (fd->lower->methods->poll != NULL)) {
        new_flags = fd->lower->methods->poll(fd->lower, new_flags,
                                                   p_out_flags);
    }

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

I have a copy of Alan's original specification for the present design of
the poll method.  Since he didn't write it for publication, I don't think
I should attach it here.  But here's a part of it:

> Additionally, if the layer has buffered data that will allow the operation 
> defined by in_flags to make progress, it will set corresponding bits in 
> out_flags. For instance, if in_flags indicates that the client (or higher 
> layer) wishes to test for read ready and the layer has input data buffered, 
> it would set the read bits in the out_flags. If that is the case, then it 
> should also suppress the calling of the next lower layer's poll() method 
> and return a value equal to that of in_flags. 

I understand that last sentence to mean that the act of returning a non-zero
value in *out_flags is mutually exclusive with changing the return value from
the caller's "view".  

In the cover letter for that document, Alan wrote:  
> As the attached memo will note, the setting of bits in *out_flags should
> correspond to the setting layer's capability to satisfy the current in_flags. 

I interpret both of those excerpts to say that the out_flags were intended to
reflect the caller's "view" of the PR_POLL_READ and PR_POLL_WRITE flags.  


The change you proposed to _pr_poll_with_poll above has the (perhaps 
unintended) consequence of making it impossible for the poll method to set the 
PR_POLL_EXCEPT bit in *out_flags.  I don't know of any code that does that 
today, but I'm not sure we should disallow it.  The present code allows it.
But perhaps it doesn't make any sense to allow it.  

Other than the PR_POLL_EXCEPT issue, your proposed change has the property
that it works properly (IINM) with poll methods that return *out_flags in 
the caller's "view", as well as with those that don't.  So, it seems like
a good change. 

Comment 2

[Wan-Teh Chang]

Attachment: The old implementation of PR_Poll (from ns/nspr20/pr/src/pthreads/ptio.c, rev. 2.26 in Netscape's /m/src cvs repository)

This is the old implementation of PR_Poll, which provides
a hint as to what the original intent of the poll method
might be.

Comment 3

[John G. Myers]

Specifying out_flags as being zero/nonzero is much less error prone than
specifying that implementations of the poll method have to return particular bits.

I don't see the utility of PR_POLL_EXCEPT.  It is not clear how a caller of
PR_Poll() should handle that bit, so if an implementation returns that value
there is a good chance the calling application will mishandle it.  Better for
the poll call to return PR_POLL_READ and/or PR_POLL_WRITE and return the
appropriate error on the subsequent read/write call.

Comment 4

[Wan-Teh Chang]

With Alan's original specification of the poll method, I think
the current PR_Poll implementation is compliant to the spec in
interpreting the out_flags argument of the poll method as
reflective of the caller's view.  This means it is the
responsibility of each I/O layer's poll method to perform
any necessary mapping of the out_flags and return value from
the next lower layer's poll method to the caller's view, as
is done at the end of ssl_Poll.  (The MyPoll method in the
nblayer.c test is not doing that, which is a bug.)  This is a
little more work on the part of the poll method, but it makes
it easier to understand the out_flags argument and the return
value of the poll method.

My proposed change does have the property that Nelson described
in comment #1.  However, given the new information that the
current PR_Poll implementation is compliant to the original
spec of the poll method, I am not inclined to changing it now.

Regarding PR_POLL_EXCEPT, I tend to agree with John's comment.
It is supposed to be the NSPR equivalent of the event reported
by the third fdset of select() or the POLLPRI flag of poll().
The problem is that the meaning of the third fdset of select()
or the POLLPRI flag of poll() is not portable.  Currently,
PR_POLL_EXCEPT is only returned by PR_Poll on Windows to report
the failure of a non-blocking connect (because Winsock's select
uses the third fdset to report the failure of a non-blocking
connect).

Comment 5

[John G. Myers]

Should we file a separate bug to have the Windows nonblocking connect code
return PR_POLL_WRITE for that condition?

Comment 6

[Wan-Teh Chang]

Yes, please file a separate bug about the Windows nonblocking
connect code's use of PR_POLL_EXCEPT.

Comment 7

[Wan-Teh Chang]

Last week Nelson found a more recent internal memo on the poll
method, which is 5 months newer than the original spec.

After I read both documents carefully, I think both documents
are silent about one point:
    If the poll method calls the next lower layer's poll method
    and the next lower layer's poll method sets some bits in
    *out_flags (which are presumably also set in the returned
    flags), it is NOT documented whether the poll method should
    map the bits in *out_flags and the returned flags to reflect
    the view of the higher layer (or client).

The current PR_Poll implementation, which is the de facto spec
of the poll method, requires that the poll method map the bits
in *out_flags and the returned flags (set by the next lower
layer's poll method) to reflect the view of the higher layer
(or client), and is therefore MORE STRICT then the original
spec.  This has two implications.

1. The poll methods that conform to the current PR_Poll
implementation also conform to the original spec.

2. The change to PR_Poll below will allow poll methods
to conform to the original spec and won't break existing
poll methods.

                    pds[index].out_flags = out_flags_read | out_flags_write;
+                   pds[index].out_flags &= ~(PR_POLL_READ | PR_POLL_WRITE);
+                   if (0 != (in_flags_read & out_flags_read))
+                       pds[index].out_flags |= PR_POLL_READ;
+                   if (0 != (in_flags_write & out_flags_write))
+                       pds[index].out_flags |= PR_POLL_WRITE;

Is it desirable to go back to the original spec?

Comment 8

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

The bug assignee didn't login in Bugzilla in the last 7 months.
:KaiE, could you have a look please?
For more information, please visit auto_nag documentation.