Closed Bug 449332 Opened 16 years ago Closed 15 years ago

SECU_ParseCommandLine does not validate its inputs

Categories

(NSS :: Libraries, defect)

3.11
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
3.12.4

People

(Reporter: nelson, Assigned: elio.maldonado.batiz)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
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.
Attachment #376184 - Flags: review?(nelson)
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
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-.
Attachment #376184 - Attachment is obsolete: true
Attachment #376360 - Flags: review?(nelson)
Attachment #376184 - Flags: review?(nelson)
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-
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.
Attachment #376360 - Attachment is obsolete: true
Attachment #376715 - Flags: review?(nelson)
Attached patch Detects duplicate options (obsolete) — Splinter Review
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 :-).
Attachment #376715 - Attachment is obsolete: true
Attachment #376944 - Flags: review?(nelson)
Attachment #376715 - Flags: review?(nelson)
Attachment #376944 - Flags: review?(nelson)
Comment on attachment 376944 [details] [diff] [review]
Detects duplicate options

Now the message should come out right :-)
Attachment #376944 - Flags: review?(nelson)
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.
Restored -m option short opetion to preserve binary compatibility with previously shipped version
Attachment #376944 - Attachment is obsolete: true
Attachment #377091 - Flags: review?(nelson)
Comment on attachment 377091 [details] [diff] [review]
Detects duplicate options

r=nelson
Attachment #377091 - Flags: review?(nelson) → review+
Target Milestone: --- → 3.12.4
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
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: