Closed Bug 110420 Opened 24 years ago Closed 24 years ago

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

Categories

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

1.0 Branch
defect

Tracking

(Not tracked)

VERIFIED FIXED
psm2.2

People

(Reporter: KaiE, Assigned: KaiE)

Details

Attachments

(1 file)

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.
Attached patch Suggested fixSplinter Review
Javi, can you please review?
Status: NEW → ASSIGNED
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
Stephane: You are right with your suggestion, I filed bug 110788.
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+
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+
checked in => fixed
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Verified.
Status: RESOLVED → VERIFIED
Product: PSM → Core
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.

Attachment

General

Created:
Updated:
Size: