125756 - nsCertTree does something wrong (crashes?) when it encounters multiple orgs from different tokens

Status: RESOLVED WONTFIX

(Core :: Security: PSM, defect, P3)

Description

[Robert Relyea]

If you have multiple orgs from different tokens, the nsCertOutliner function
build and invalid nsOutliner array which causes debug build to crash and
non-debug builds to display empty entries.

This is because the numOrgs calculation and the actual outliner row layout code
are inconsistant. The numOrgs calculator assumes that there is a separate orgs
line for certs from the same organization, but different tokens. The outliner
layout assumes all certs from the same organization, reguardless of token, would
be displayed in the under a single organization entry. The confusion only really
comes when the two different tokens have their organization entry sorted
together in the list.

Also, the reason this code isn't just blowing up is there is protective code in
the Outliner which prevents the loop from continuing if you have ran out of
certs. Unfortunately it leaves parts of the outliner array uninitialized, and
leaves an incorrect calculation for the number of orgs (causing the extra empty
row).

bob

Comment 1

[Robert Relyea]

Attachment: Fix crash in cert display

This patch fixes the crash but 1) distinguishing between certs with the same
org but different tokens, and 2) adjusting the outliner count on exit of the
loop so that the outliner structure is consistent, even if the calculated
values for it is not.

Comment 2

[Robert Relyea]

updating priority so it shows up in ssaux's radar. (the bug includes a patch, so
it really just needs to be reviewed).

Comment 3

[Stephane Saux]

javi can you review?

Comment 4

[Javier Delgadillo]

Comment on attachment 69748 [details] [diff] [review]
Fix crash in cert display

what is j supposed to be?  I looked at the original source and can't tell what
it's supposed to be counting.  Perhaps mcgreer can tell us since I believe this
is his code originally.

Comment 5

[Javier Delgadillo]

adding mcgreer to cc list since he is the original author of the code.

Comment 6

[Robert Relyea]

j is the current cert index.

The Outliner calculates how many display slots it needs to show all the certs.
When the cert count exceeds that number of display slots, the loop bails. This
is an exceptional case, and happens only when there was some problem calculating
the number of certs. Unfortunately the rest of the code assumes that all the
organizations have been processed, which isn't true if we bail from the loop early.

This patch contains two fixes: 1) paranoic fixes to have the code work if there
is an exceptional 'bail in the middle' event (by reducing the number of orgs to
only those that were processed). 2) Correct the loop operation so that it
calculates organization groups consistently with the numorgs call so that we
shouldn't have to bail early.

bob

Comment 7

[Javier Delgadillo]

Comment on attachment 69748 [details] [diff] [review]
Fix crash in cert display

Thanks for clarification.
Looks good.
r=javi

Comment 8

[John Unruh]

Adding patch keyword.

Comment 9

[Kai Engert [:KaiE:]]

Bob, unfortunately your patch has pretty much rotted.

It obviously has been forgotten, I never saw this bug before, but I have changed
the code in that area multiple times. The most important change is the sort order.

In the past, at the time you were producing your patch, sort criteria 1 was
"token", and second criteria was "issuer org". The new sort order reverses that,
and details are explained in bug 121906 comment 5.

However, the function you changed is in general still using the same logic, so
your patch might still be correct.

Comment 10

[Kai Engert [:KaiE:]]

-> me

Comment 11

[Kai Engert [:KaiE:]]

changing obsolete psm* target to --- (unspecified)

Comment 12

[Robert Kaiser]

Is this still relevant? If so, should we drive the patch in? Do we have crash signatures?
If it's not relevant any more, can we close it out?

Comment 13

[Dana Keeler (she/her) [:keeler]]

This might still be relevant. nsCertOutliner got transformed into nsCertTree, so the code in question still exists in nsCertTree::UpdateUIContents. If someone takes the time to figure out how to reproduce this and demonstrates that it is a problem, it might be worth fixing directly, but I'm inclined to go with "this whole thing should be written in JS anyway", which is an in-progress task (we're starting with the cert viewer).

Comment 14

[Andrei Purice]

Hey Robert,
Can you still reproduce this or should we close the issue?

Comment 15

[Robert Relyea]

We should close this issue. I don't have the token that was causing the issue anymore. If no one else has tripped over this in 20 years, it it's still there, it's clearly not bothering anyone (you need to have multiple tokens attached to your browser to cause this).