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)
NSPR
NSPR
Tracking
(Not tracked)
RESOLVED
FIXED
4.9.3
People
(Reporter: michal, Assigned: michal)
References
Details
Attachments
(3 files, 3 obsolete files)
941 bytes,
patch
|
Details | Diff | Splinter Review | |
3.45 KB,
patch
|
Details | Diff | Splinter Review | |
2.17 KB,
patch
|
michal
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
Pushed to tryserver: https://tbpl.mozilla.org/?tree=Try&rev=020a71547dab
Attachment #664738 -
Flags: review?(wtc)
Comment 2•12 years ago
|
||
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
Assignee | ||
Comment 3•12 years ago
|
||
(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•12 years ago
|
||
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+
Assignee | ||
Comment 5•12 years ago
|
||
Thanks for a quick review. Does this patch need sr?
Attachment #664738 -
Attachment is obsolete: true
Comment 6•12 years ago
|
||
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
Comment 7•12 years ago
|
||
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)
Updated•12 years ago
|
Attachment #664766 -
Attachment description: patch v2.1 → patch v2.1, by Michal Novotny
Assignee | ||
Updated•12 years ago
|
Attachment #664767 -
Flags: review?(michal.novotny) → review+
Comment 8•12 years ago
|
||
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
Updated•12 years ago
|
Status: NEW → RESOLVED
Closed: 12 years ago
Priority: -- → P1
Resolution: --- → FIXED
Comment 9•12 years ago
|
||
Michal: I found that PR_PopIOLayer has a similar bug. Please review. Thanks.
Attachment #665787 -
Flags: review?(michal.novotny)
Assignee | ||
Updated•12 years ago
|
Attachment #665787 -
Flags: review?(michal.novotny) → review+
Assignee | ||
Comment 10•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d08437736a60
Comment 11•12 years ago
|
||
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!
Updated•11 years ago
|
blocking-basecamp: ? → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•