Closed Bug 794316 Opened 12 years ago Closed 12 years ago

PR_PushIOLayer doesn't push layer on the top correctly

Categories

(NSPR :: NSPR, defect, P1)

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: michal, Assigned: michal)

References

Details

Attachments

(3 files, 3 obsolete files)

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.
Attached patch fix (obsolete) — Splinter Review
Pushed to tryserver: https://tbpl.mozilla.org/?tree=Try&rev=020a71547dab
Attachment #664738 - Flags: review?(wtc)
Blocks: 777445
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.
blocking-basecamp: --- → ?
Target Milestone: --- → 4.9.3
(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 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.
Attachment #664738 - Flags: review?(wtc) → review+
Attached patch patch v2 (obsolete) — Splinter Review
Thanks for a quick review. Does this patch need sr?
Attachment #664738 - Attachment is obsolete: true
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
Attachment #664756 - Attachment is obsolete: true
Attached patch Add a regression test (obsolete) — Splinter Review
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.
Attachment #664767 - Flags: review?(michal.novotny)
Attachment #664766 - Attachment description: patch v2.1 → patch v2.1, by Michal Novotny
Attachment #664767 - Flags: review?(michal.novotny) → review+
Blocks: 795136
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
Attachment #664767 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 12 years ago
Priority: -- → P1
Resolution: --- → FIXED
Michal: I found that PR_PopIOLayer has a similar bug.
Please review. Thanks.
Attachment #665787 - Flags: review?(michal.novotny)
Attachment #665787 - Flags: review?(michal.novotny) → review+
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!
blocking-basecamp: ? → ---
You need to log in before you can comment on or make changes to this bug.