The default bug view has changed. See this FAQ.

pk12util has duplicate options letters

RESOLVED FIXED in 3.12.2

Status

NSS
Tools
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Elio Maldonado)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

1.32 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review
While reviewing a patch for bug 157942 today, I discovered that a nasty bug
got into pk12util for NSS 3.12 as part of the checkins for bug 401928.

The checkins for that bug added two new command line options using letters
that were already in use.  There are now *TWO* -k and *TWO* -K options 
defined for pk12util.  See 
<http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/cmd/pk12util/pk12util.c&rev=1.39&mark=944-945,956-957#939>

This needs to be fixed, ASAP, with the introduction of new option letters.
Also, the behavior of those option letters needs to be TESTED.  Those 
options were obviously untested before, because they could not have worked.

The work on bug 157942 is blocked by this. See also bug 449332.
(Assignee)

Comment 1

9 years ago
I propose using long options for the duplicate -k and -K arguments
   .....
    { /* opt_KeyLength	       */ '0', PR_TRUE,  0, PR_FALSE, "key_len" },
    { /* opt_CertKeyLength     */ '0', PR_TRUE,  0, PR_FALSE, "cert_key_len" }
};
Since I'm blocked bug 157942 I may as well work on this one as well. 

Assignee: nobody → emaldona
Please check one more thing about pk12util. 
Put the softoken in FIPS mode, and then test to see if the -l (list) option
works.
Summary: pk12util have duplicate options letters → pk12util has duplicate options letters
(Assignee)

Comment 3

9 years ago
Created attachment 337877 [details] [diff] [review]
patch fixes duplicate options

Using long options as to resolve the options letters collision.
Attachment #337877 - Flags: review?(nelson)
(Assignee)

Updated

9 years ago
Target Milestone: 3.12.1 → 3.12.2
(Reporter)

Updated

9 years ago
Attachment #337877 - Flags: review?(nelson) → review-
Comment on attachment 337877 [details] [diff] [review]
patch fixes duplicate options

r-   I'm sorry this review took so long.

>     { /* opt_CertCipher	       */ 'C', PR_TRUE,  0, PR_FALSE },
>-    { /* opt_KeyLength	       */ 'k', PR_TRUE,  0, PR_FALSE },
>-    { /* opt_CertKeyLength     */ 'K', PR_TRUE,  0, PR_FALSE }
>+    { /* opt_KeyLength         */ '0', PR_TRUE,  0, PR_FALSE, "key_len" },
>+    { /* opt_CertKeyLength     */ '0', PR_TRUE,  0, PR_FALSE, "cert_key_len" }
> };

This has the effect of defining both of these new options has being represented by
the same single character option character, namely the ASCII zero '0' character.
So, this patch does ensure that these two options don't duplicate the other existing
-k and -K options, but it has the effect of creating a new duplicate -0 option.

Note that it is possible for an option to have both a single character form 
and a multiple-character form.  This patch does just that for these two options.
If you want an option to have only the multiple character form, the single character
"flag" value must be binary zero (not ASCII zero digit).  It would be correct to 
specify simply  0,  or '\0', but '0' is the ASCII zero digit.

Having said all that, I see that this is pk12util, which (unlike certutil) is 
nowhere near running out of single-character options yet.  So, please give these
new options both unique single-character options and also multi-character options.
(Assignee)

Comment 5

9 years ago
Created attachment 340590 [details] [diff] [review]
solve duplicate options with long and short options

Using -m and -n the as the one-letter versions of -key_len and cert_key_len long options, respectively.
Attachment #337877 - Attachment is obsolete: true
Attachment #340590 - Flags: review?(nelson)
Comment on attachment 340590 [details] [diff] [review]
solve duplicate options with long and short options



>+    FPS "\t\t [-c key_cipher] [-C cert_cipher] [m|--key_len keyLen] [n|--cert_key_len certKeyLen]\n");

Put a dash before the new m and n, and spaces around the | characters, e.g.
[-m | --key_len ...

Do that for both the new m and new n options.
Then r=nelson
Attachment #340590 - Flags: review?(nelson) → review+
(Assignee)

Comment 7

9 years ago
Changes checked in. Changed: mozilla/security/nss/cmd/pk12util/pk12util.c
new revision: 1.40; previous revision: 1.39

Updated

9 years ago
Blocks: 459298
(Assignee)

Updated

9 years ago
Depends on: 458287
(Assignee)

Updated

9 years ago
No longer depends on: 458287
(Assignee)

Updated

9 years ago
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
No longer blocks: 459298
You need to log in before you can comment on or make changes to this bug.