140302 - hang when collapsing some ssl-security details

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect, P3)

Description

[Daniel de Wildt]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.0rc1)
Gecko/20020417
BuildID:    2002041711

When I view the detailed security information of ssl-secured page and I collapse
the branch "Subject Public Key Info" in the "Certificate Fields"  -Information
field, Mozilla hangs with a cpu usage of 100%.

Reproducible: Always
Steps to Reproduce:
1. Enter a ssl-secured site (https://www.verisign.com)
2. Select the page information
3. Choose the security tab
(or select key-symbol at the right-bottom of the window)
4. Select "View"
5. In the new window choose the tab "Details"
6. Select the key "Subject Public Key Algorithm" or "Subject's Public Key" in
the branch "Subject Public Key Info" in "Certificate Fields"
7. Collapse these branch with the information by selecting the arrow of "Subject
Public Key Info"

Actual Results:  cpu usage is nearly 100% and nothing else happens

Expected Results:  The selected branch in the tree should be collapsed

Comment 1

[Daniel de Wildt]

It could happen that in the "Certificate Viewer" you must first select "Subject
Public Key Info" and THEN collapse this tree to reproduce the bug.

Comment 2

[Mitchell Stoltz (not reading bugmail)]

->PSM

Comment 3

[John Unruh]

Confirming with the 5/20 Win2000 trunk build.

Comment 4

[Stephane Saux]

kai for investigation.
can reproduce with Kai 05/10 build.

Comment 5

[Kai Engert [:KaiE:]]

Does not happen on Linux, will try Windows.

Comment 6

[Kai Engert [:KaiE:]]

I take my previous statement back, I can reproduce on both Windows and Linux.

The endless loop seems to be caused by a behaviour of
nsNSSASN1Tree::GetParentIndex. It gets called with an parameter value of "1". It
calls GetParentOfObjectAtIndex, which also returns 1, and we end up being called
again with a param of 1.

Without knowing much details about the implementation yet, I'd naively assume
that we should not report 1 as being the parent of 1.

Comment 7

[alge]

*** Bug 158725 has been marked as a duplicate of this bug. ***

Comment 8

[Kai Engert [:KaiE:]]

Attachment: Patch v1

Comment 9

[Kai Engert [:KaiE:]]

I think a hang is as bad as a crash.

I tried to understand what is going on, but the existing code gave me a hard
time. I eventually gave up.

The attached patch is a re-implementation of the broken tree view code.

Besides that it fixes this bug, I believe it is also more efficient. Instead of
iterating the lower data structure over and over again, we now create a helper
data structure that simplifies things a lot.


Javi, can you please review?

Comment 10

[Javier Delgadillo]

Comment on attachment 95398 [details] [diff] [review]
Patch v1

 >@@ -252,20 +246,11 @@
> NS_IMETHODIMP 
> nsNSSASN1Tree::IsContainerOpen(PRInt32 index, PRBool *_retval)
> {
>-  nsCOMPtr<nsIASN1Object> object;
>-  nsCOMPtr<nsIASN1Sequence> sequence;
>-
>-  nsresult rv = GetASN1ObjectAtIndex(index, mASN1Object,
>-                                     getter_AddRefs(object));
>-  if (NS_FAILED(rv))
>-    return rv;
>+  myNode *n = FindNodeFromIndex(index);
>+  if (!n)
>+    return NS_ERROR_FAILURE;
> 
>-  sequence = do_QueryInterface(object);
>-  if (sequence == nsnull) {
>-    *_retval = PR_FALSE;
>-  } else {
>-    sequence->GetShowObjects(_retval);
>-  }
>+  n->seq->GetShowObjects(_retval);
This code should keep the sanity check and make sure the passed in index is
actually 
a sequence (adding an assert that calls IsContainer on the index may be
enough).  If for some reason an index gets passed in that doesn't have a
sequence or somehow your structures falls out of sync with what the layout
engine thinks, 
you're going to crash here.


Other than that looks good.  r=javi

(Sorry for doing such a bad job on the original code, but I wrote this when
outliner was brand spanking new.)

Comment 11

[Kai Engert [:KaiE:]]

Javi, no worries, I agree that I have the big advantage the tree view interface
is now stable. In addition it seems to be required now to implement the
HasNextSibling method, and the new implementation will also use more memory,
while the previous did not allocate more than the basic data structure.

Re the sanity check: In InitNodesRecursively in myNode->seq a reference to the
sequence will be cached if the node is a sequence and has childs, otherwise it's
set to null. Function FindNodeFromIndex is doing the sanity check by checking
that myNode->seq is actually set.

Comment 12

[Javier Delgadillo]

I'm talking about the variable n you set to the return value of FindNodeFromIndex.

You're assuming the index passed in corresponds to an index that is a container.
 If passed in index actually points to the index associated with the NotBefore
(which is not a container), n->sequence will in fact be NULL, no?  If so, then
this line:

+  n->seq->GetShowObjects(_retval);

will try to de-reference a NULL pointer and subsequently crash.  I don't think
FindNodeFromIndex guarantees the return value has a sequence.

Comment 13

[Kai Engert [:KaiE:]]

You are right, now I see. Thanks for catching this. New patch coming up.

Comment 14

[Kai Engert [:KaiE:]]

Attachment: Patch v2

Patch has added sanity check in IsContainerOpen.

Comment 15

[Kai Engert [:KaiE:]]

Comment on attachment 95482 [details] [diff] [review]
Patch v2

carrying forward r=javi

Comment 16

[Brian Ryner (not reading)]

Comment on attachment 95482 [details] [diff] [review]
Patch v2

General comments:

Method names seems to be a mixture of first-letter-lowercase (clearNodes) and
uppercase (GetRowCount).  I'd suggest settling on one, probably the latter
since it's the prevailing style.


>--- mozilla/security/manager/pki/src/nsASN1Tree.cpp	15 May 2002 18:54:54 -0000	1.9
>+++ mozilla/security/manager/pki/src/nsASN1Tree.cpp	15 Aug 2002 23:32:30 -0000
>+PRInt32 nsNSSASN1Tree::CountVisibleNodes(myNode *n)
>+{
>+  if (!n)
>+    return 0;
>+
>+  myNode *walk = n;
>+  int count = 0;

I'd use PRInt32 for |count| for compatibility with the return type.

I'm not familiar with this code so I can't really tell if the tree logic is
correct but it looks fine.  sr=bryner with the above two changes.

Comment 17

[Kai Engert [:KaiE:]]

Attachment: Patch v3

New patch with bryner's suggestions.

Comment 18

[Kai Engert [:KaiE:]]

Comment on attachment 95859 [details] [diff] [review]
Patch v3

carrying forward reviews

Comment 19

[John Unruh]

*** Bug 162962 has been marked as a duplicate of this bug. ***

Comment 20

[Kai Engert [:KaiE:]]

Fixed on trunk.

Comment 21

[John Unruh]

Verified on trunk. I cannot make the browser hang.