449334 - pk12util has duplicate options letters

Status: RESOLVED FIXED

(NSS :: Tools, defect)

Description

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 1

[Elio Maldonado]

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. 

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Please check one more thing about pk12util. 
Put the softoken in FIPS mode, and then test to see if the -l (list) option
works.

Comment 3

[Elio Maldonado]

Attachment: patch fixes duplicate options

Using long options as to resolve the options letters collision.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 5

[Elio Maldonado]

Attachment: 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.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 7

[Elio Maldonado]

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