Closed
Bug 110420
Opened 23 years ago
Closed 23 years ago
Client Auth & Cert Picker will not work for > 6 certificates, Token not for > 16
Categories
(Core Graveyard :: Security: UI, defect, P2)
Tracking
(Not tracked)
VERIFIED
FIXED
psm2.2
People
(Reporter: KaiE, Assigned: KaiE)
Details
Attachments
(1 file)
7.61 KB,
patch
|
javi
:
review+
shaver
:
superreview+
|
Details | Diff | Splinter Review |
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•23 years ago
|
||
Comment 3•23 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•23 years ago
|
||
Stephane: You are right with your suggestion, I filed bug 110788.
Comment 5•23 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•23 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•23 years ago
|
||
checked in => fixed
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•