SECU_ParseCommandLine takes, as its fourth argument, an array of secuCommandFlag structures. It does not validate the contents of that array. It does not detect that the array contains duplicate definitions for some command letters, of which all but the first will never be honored. It should check for such duplicates and ASSERT that no duplicates exist. This is arguably an enhancement request, but given that we have actually had such duplicates checked in, I consider the failure to detect these duplicates to be a bug in SECU_ParseCommandLine.
Created attachment 376184 [details] [diff] [review] Detects duplicate options and commands Added a function (and two macros) to detect duplicate command line options. The test for duplicates is invoked via assertions. This a crude linear search over the commands and options arrays with no attempt to be informative to the calling tool. It is invoked via assertion. Found such duplicate in pk12util.
Use of temporary variables in HasNoDuplicates char t = a[i].flag; and char *target = a[i].longform; is unnecessary and doesn't even add to readability. Defined "friendly" macros +#define HasNoDuplicateCommands(c) HasNoDuplicates((c)->commands,(c)->numCommands) +#define HasNoDuplicateOptions(c) HasNoDuplicates((c)->options,(c)->numOptions) but below called the function directly + PR_ASSERT(HasNoDuplicates(cmd->commands, cmd->numCommands)); + PR_ASSERT(HasNoDuplicates(cmd->options, cmd->numOptions));
Assignee: nobody → emaldona
Status: NEW → ASSIGNED
Created attachment 376360 [details] [diff] [review] Elio's patch, ignoring white space changes Elio, Apparently you used a text editor that removes trailing white space on all lines of the files it edits. That causes your patches to appear to be HUGE, and hides the changes you really intend to make. I used cvs diff -puw to produce a new version of your patch, without all the white space changes. It is MUCH smaller than the original patch. I will review it. Please see if you can change the behavior of your editor so that it no longer makes all those unnecessary white space changes. I think patches with all those unnecessary white space changes will generally get r-.
Comment on attachment 376360 [details] [diff] [review] Elio's patch, ignoring white space changes There are several issues with this patch. 1) Function HasNoDuplicates is only called from with the PR_ASSERT() macro, which means that it will not be called in optimized builds. So, it should be enclosed inside an #ifdef to avoid building it when it will not be used. The correct ifdef to use is: #if defined(DEBUG) || defined(FORCE_PR_ASSERT) 2) Function HasNoDuplicates seems to assume that short, 1-character flag names and longer "longform" names are mutually exclusive. It checks for duplicates of one, or the other, but not both. They are not mutually exclusive. It's quite OK for an option to have both a long and a short form name. This function should detect duplicates of both forms, without regard to the presence or absence of the other form. 3) This patch defines two macros, named HasNoDuplicateCommands and HasNoDuplicateOptions, but does not use them anywhere. Please just eliminate them. I think the direct invocations of HasNoDuplicates are fine. 4) This patch mysteriously adds the string "ssbb" to one of the usage messages. 5. Please don't use option '0' as the short flag name for opt_CertKeyLength. '0' and 'O' are too easily confused. Please find another lower case letter to use, and be sure it appears in the usage message.
Attachment #376360 - Flags: review?(nelson) → review-
Created attachment 376715 [details] [diff] [review] Detects duplicate options and fixes duplicate option in pk12util Addresses comments in review. Note that we allow duplicated short option of '0' as long as the long forms are different. This occurs in certutil where some of the new long form options have no corresponding short form.
Created attachment 376944 [details] [diff] [review] Detects duplicate options Fix some errors pointed out by Nelson. Fixed an inaccurate comment on secutil.c, it's 0 ('\0') the value used with long options that don't have a short version and not '0'. pk12util now respect the 80-char limit and both code and the output of the usage message. Notice also that the --key-len and --cert-key-len now have only the long form as -m and -q are hardly mnemonic in this two cases. These are likely seldom used and hopefully in this short time no automation scripts have become addicted to -m :-).
Comment on attachment 376944 [details] [diff] [review] Detects duplicate options Now the message should come out right :-)
Comment on attachment 376944 [details] [diff] [review] Detects duplicate options Elio, this patch is good, except for the removal of the -m flag for the key_len option. Since -m appeared in NSS 3.12.3, which is now installed on customers' systems, we cannot remove it. So, please put that -m option back. It's OK for that option to have two alternative ways of being specified.
Attachment #376944 - Flags: review?(nelson) → review-
Ah, some inner voice told me earlier that dropping the -m would not pass muster but I couldn't resist trying. New patch coming soon.
Created attachment 377091 [details] [diff] [review] Detects duplicate options Restored -m option short opetion to preserve binary compatibility with previously shipped version
Comment on attachment 377091 [details] [diff] [review] Detects duplicate options r=nelson
Attachment #377091 - Flags: review?(nelson) → review+
Fix commited /cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v <-- secutil.c new revision: 1.97; previous revision: 1.96 done Checking in pk12util/pk12util.c; /cvsroot/mozilla/security/nss/cmd/pk12util/pk12util.c,v <-- pk12util.c new revision: 1.42; previous revision: 1.41 done
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.