110420 - Client Auth & Cert Picker will not work for > 6 certificates, Token not for > 16

Status: VERIFIED FIXED

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

Description

[Kai Engert [:KaiE:]]

Client authentication dialog will not work when having more than 6 personal
certificates.

Cert Picker does work when having more then 6 personal certs.

Token select dialog will not work when having more than 16 tokens.

I was not aware that DialogParamBlocks allow storing of *some* strings, but does
not grow automatically. I didn't see any code that allocates the number of
strings that should be stored in such param blocks, and I didn't do it in in the
new code I wrote.

In my eyes the class DialogParamBlocks has a dangerous behaviour. It should
either provide no default space at all, forcing the programmer to allocate the
needed space (and to think about it), or it should grow automatically!

While searching for this bug, I decided to make the information assembling code
for client auth and cert picker dialog more failsafe.

Comment 1

[Kai Engert [:KaiE:]]

Attachment: Suggested fix

Comment 2

[Kai Engert [:KaiE:]]

Javi, can you please review?

Comment 3

[Stephane Saux]

Kai,
on a related note, when will we use the same dialog for both the cert picker and
the client auth chooser?

Comment 4

[Kai Engert [:KaiE:]]

Stephane: You are right with your suggestion, I filed bug 110788.

Comment 5

[Javier Delgadillo]

Comment on attachment 58076 [details] [diff] [review]
Suggested fix

r=javi

One nit pick.  Take it or leave as you wish.  I prefer the style of 

 ++CertsToUse, node=CERT_LIST_NEXT(node) 

as the 3rd statement inside the for loop parens since CertsToUse is what
you're iterating over as well as node.


>-    for (i = 0, node = CERT_LIST_HEAD(certList);
>-         !CERT_LIST_END(node, certList);
>-         ++i, node = CERT_LIST_NEXT(node)
>+    PRInt32 CertsToUse;
>+
>+    for (CertsToUse = 0, node = CERT_LIST_HEAD(certList);
>+         !CERT_LIST_END(node, certList) && CertsToUse < nicknames->numnicknames;
>+         node = CERT_LIST_NEXT(node)
>         )
>     {
...
>+        ++CertsToUse;
>       }
>     }

Comment 6

[Kai Engert [:KaiE:]]

Javi: Thanks. However, I'd like to keep the current code. The ++CertsToUse is
not executed all the time, but only when a we tempCert could be allocated. If I
move the incrementation to the loop header, I'd have to add a --CertsToUse to
the failure case.

Comment 7

[Mike Shaver (:shaver emeritus)]

Comment on attachment 58076 [details] [diff] [review]
Suggested fix

I'd like to see the SetNumberStrings thing replaced with a fix like that
discussed in bug
111552, but I can live with this in the meantime.

sr=shaver

Comment 8

[Kai Engert [:KaiE:]]

checked in => fixed

Comment 9

[John Unruh]

Verified.