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

VERIFIED FIXED in psm2.2

Status

Core Graveyard
Security: UI
P2
normal
VERIFIED FIXED
16 years ago
a year ago

People

(Reporter: kaie, Assigned: kaie)

Tracking

1.0 Branch
psm2.2

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

16 years ago
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.
(Assignee)

Comment 1

16 years ago
Created attachment 58076 [details] [diff] [review]
Suggested fix
(Assignee)

Comment 2

16 years ago
Javi, can you please review?
Status: NEW → ASSIGNED

Comment 3

16 years ago
Kai,
on a related note, when will we use the same dialog for both the cert picker and
the client auth chooser?
Priority: -- → P2
Target Milestone: --- → 2.2
(Assignee)

Comment 4

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

Comment 5

16 years ago
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;
>       }
>     }
Attachment #58076 - Flags: review+
(Assignee)

Comment 6

16 years ago
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 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
Attachment #58076 - Flags: superreview+
(Assignee)

Comment 8

16 years ago
checked in => fixed
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 9

16 years ago
Verified.
Status: RESOLVED → VERIFIED

Updated

13 years ago
Component: Security: UI → Security: UI
Product: PSM → Core

Updated

9 years ago
Version: psm2.1 → 1.0 Branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.