794316 - PR_PushIOLayer doesn't push layer on the top correctly

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P1)

Description

[Michal Novotny [:michal]]

When pushing a layer on the top of the stack PR_PushIOLayer swaps the new layer with the layer on the top, but it doesn't change the "higher" pointer of the next layer, so it points to the top layer instead of the second layer. Further manipulation with the stack (like inserting layer in the middle) can break the chain completely.

Comment 1

[Michal Novotny [:michal]]

Attachment: fix

Pushed to tryserver: https://tbpl.mozilla.org/?tree=Try&rev=020a71547dab

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Michal, please post the code snippit that demonstrates the problem.

Wan-Teh, I find the NSPR layer mechanism a little difficult to understand, so I am not sure if we're doing something wrong or if this is really a bug in NSPR. In particular, I don't understand the distinction between "old style layers" and "new style layers" and whether we are using the right APIs.

If this is a bug, then we'd like to have the fix in the NSPR release that accompanies the NSS 3.14 release if at all possible. Or, we can take the fix privately in mozilla-central while we wait for the next release.

Basically, we are trying to push an IO layer between two layers already in the stack, e.g. between the SSL and NSPR layers.

Comment 3

[Michal Novotny [:michal]]

(In reply to Brian Smith (:bsmith) from comment #2)
> Michal, please post the code snippit that demonstrates the problem.

Here is the example. First we push 2 layers on the top and we end up with wrong higher pointer at the nspr level, because it isn't updated when psm is added and is swapped with ssl.

PR_PushIOLayer(fd, PR_TOP_IO_LAYER, "ssl");
PR_PushIOLayer(fd, PR_TOP_IO_LAYER, "psm");

layer  lower  higher
--------------------
psm    ssl    NULL
ssl    nspr   psm
nspr   NULL   psm 


Then inserting a new layer between ssl and nspr damages the chain completely.

PR_PushIOLayer(fd, PR_NSPR_IO_LAYER, "netmon");

layer  lower  higher
--------------------
psm    netmon NULL
ssl    nspr   psm
netmon nspr   psm
nspr   NULL   netmon


As you can see, ssl layer is now completely lost.


> Wan-Teh, I find the NSPR layer mechanism a little difficult to understand,
> so I am not sure if we're doing something wrong or if this is really a bug
> in NSPR. In particular, I don't understand the distinction between "old
> style layers" and "new style layers" and whether we are using the right APIs.

See bug #30914

Comment 4

[Wan-Teh Chang]

Comment on attachment 664738 [details] [diff] [review]
fix

Michal: Thank you for the patch. I think your fix is correct.
Please fix the curly brace style, and add an assertion:

    if (fd->lower) {
        PR_ASSERT(fd->lower->higher == stack);
        fd->lower->higher = fd;
    }

bsmith: a new-style stack is one which has a dummy
layer at the top, so that the confusing swapping of
PRFileDesc structures won't be done. In a new-style
stack, the top layer is hardcoded to be the dummy
layer, so a new layer can never be added to the top.
The dummy top layer has the special layer id
PR_IO_LAYER_HEAD. For more info, see bug 30914.

Comment 5

[Michal Novotny [:michal]]

Attachment: patch v2

Thanks for a quick review. Does this patch need sr?

Comment 6

[Wan-Teh Chang]

Attachment: patch v2.1, by Michal Novotny

Michal: I found that prlayer.c has inconsistent curly brace styles.
Your original patch used the prevalent style in that file, so I
changed it back. Sorry about that.

Patch checked in on the NSPR trunk (NSPR 4.9.3).

Checking in prlayer.c;
/cvsroot/mozilla/nsprpub/pr/src/io/prlayer.c,v  <--  prlayer.c
new revision: 3.22; previous revision: 3.21
done

Comment 7

[Wan-Teh Chang]

Attachment: Add a regression test

We can work around this bug by using new-style stacks:
http://mxr.mozilla.org/security/ident?i=PR_CreateIOLayer

But that could be a lot of work.

I added a regression test. Please review. Thanks.

Comment 8

[Wan-Teh Chang]

Attachment: Add a regression test v1.1

Also add the pushtop test to the test scripts.

Patch checked in on the NSPR trunk (NSPR 4.9.3).

Checking in Makefile.in;
/cvsroot/mozilla/nsprpub/pr/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.70; previous revision: 1.69
done
RCS file: /cvsroot/mozilla/nsprpub/pr/tests/pushtop.c,v
done
Checking in pushtop.c;
/cvsroot/mozilla/nsprpub/pr/tests/pushtop.c,v  <--  pushtop.c
initial revision: 1.1
done
Checking in runtests.pl;
/cvsroot/mozilla/nsprpub/pr/tests/runtests.pl,v  <--  runtests.pl
new revision: 1.9; previous revision: 1.8
done
Checking in runtests.sh;
/cvsroot/mozilla/nsprpub/pr/tests/runtests.sh,v  <--  runtests.sh
new revision: 1.11; previous revision: 1.10
done

Comment 9

[Wan-Teh Chang]

Attachment: Fix a similar bug in PR_PopIOLayer

Michal: I found that PR_PopIOLayer has a similar bug.
Please review. Thanks.

Comment 10

[Michal Novotny [:michal]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d08437736a60

Comment 11

[Wan-Teh Chang]

Comment on attachment 665787 [details] [diff] [review]
Fix a similar bug in PR_PopIOLayer

Patch checked in on the NSPR trunk (NSPR 4.9.3).

Checking in pr/src/io/prlayer.c;
/cvsroot/mozilla/nsprpub/pr/src/io/prlayer.c,v  <--  prlayer.c
new revision: 3.23; previous revision: 3.22
done
Checking in pr/tests/pushtop.c;
/cvsroot/mozilla/nsprpub/pr/tests/pushtop.c,v  <--  pushtop.c
new revision: 1.2; previous revision: 1.1
done

Michal: NSPR in mozilla-central needs to be updated using the
procedure documented at
https://developer.mozilla.org/en-US/docs/Updating_NSPR_or_NSS_in_mozilla-central

It is not necessary to undo your push to mozilla-inbound, but
next time please ask one of us to push a new NSPR CVS tag for
you. Thanks!

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d08437736a60