265504 - cmsutil core dumps with invalid option set.

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[Jason Reid]

If cmsutil is not given a file for batch mode, the command core dumps.

$ uname -a
SunOS onias 5.10 s10_68 sun4u sparc SUNW,Sun-Blade-100
$ /usr/sfw/bin/cmsutil -D -b -i          
cmsutil: unable to open "Segmentation Fault(coredump)
$ file core
core:           ELF 32-bit MSB core file SPARC Version 1, from 'cmsutil'
$ pkginfo -l SUNWtlsu
   PKGINST:  SUNWtlsu
      NAME:  Network Security Services Utilities
  CATEGORY:  system
      ARCH:  sparc
   VERSION:  3.9.3,REV=2004.07.22.04.51
   BASEDIR:  /
    VENDOR:  Sun Microsystems, Inc.
      DESC:  Network Security Services Utilities Programs
    PSTAMP:  shame20040722045210
  INSTDATE:  Sep 24 2004 14:44
   HOTLINE:  Please contact your local service provider
    STATUS:  completely installed
     FILES:       21 installed pathnames
                   3 shared pathnames
                   4 directories
                  16 executables
               21279 blocks used (approx)

I have reproduced this on both SPARC and x86 systems.

Comment 1

[Julien Pierre]

Jason, thanks for the report. If you can, in the future, please report the stack
of crashes using "dbx <progname core" and the "where" command.

In this case the crash is due to lack of validation on the value argument for -i 

(dbx) where     
  [1] strlen(), at 0xfffffd7fff2aa2b3 
  [2] _ndoprnt(), at 0xfffffd7fff2d5173 
  [3] fprintf(), at 0xfffffd7fff2d6042 
=>[4] main(argc = 4, argv = 0xfffffd7ffffff788), line 1296 in "cmsutil.c"
(dbx) list 1290,1300
 1290               }
 1291               break;
 1292           case 'i':
 1293               inFile = PR_Open(optstate->value, PR_RDONLY, 00660);
 1294               if (inFile == NULL) {
 1295                   fprintf(stderr, "%s: unable to open \"%s\" for reading\n",
 1296                           progName, optstate->value);
 1297                   exit(1);
 1298               }
 1299               break;
 1300   
(dbx) p *optstate   
*optstate = {
    option   = 'i'
    value    = (nil)
    internal = 0x43cf30
}
(dbx) 

Comment 2

[Julien Pierre]

Attachment: fix coredump on -i without file

You would think getopt would not provide a NULL ptr in optstate->value given
the "i:" definition in the parser which means it requires an argument. sigh.
We should probably review all the other arguments, but I only have time to fix
this one today.

Comment 3

[Saul Edwards]

Comment on attachment 162894 [details] [diff] [review]
fix coredump on -i without file

I seem to remember fixing this for some of our other commands... I'm sure this
problem is rampant.

If the option parser didn't provide the NULL pointer, how else would we know of
an error state in PL_GetOpt?

Comment 4

[Julien Pierre]

Saul, thanks for the review.

Regarding the 2nd question - the getopt parser call could fail . But I guess we
may not know about specific failures then.

It sounds like we need to check all the tools that we ship for this problem in
3.10 .

Comment 5

[Wan-Teh Chang]

Comment on attachment 162894 [details] [diff] [review]
fix coredump on -i without file

> 	case 'i':
> 	    inFile = PR_Open(optstate->value, PR_RDONLY, 00660);
> 	    if (inFile == NULL) {
>-		fprintf(stderr, "%s: unable to open \"%s\" for reading\n",
>-			progName, optstate->value);
>+                if (optstate->value) {
>+                    fprintf(stderr, "%s: unable to open \"%s\" for reading\n",
>+                            progName, optstate->value);
>+                } else {
>+                    fprintf(stderr, "-i option requires filename argument\n");
>+                }
> 		exit(1);
> 	    }
> 	    break;

You should check for a null optstate->value before
the PR_Open call.  It is bad to pass a null pointer
as the first argument to PR_Open.

Comment 6

[Julien Pierre]

Attachment: check the ptr before PR_Open

Comment 7

[Julien Pierre]

Thanks, Wan-Teh.

Comment 8

[Wan-Teh Chang]

Comment on attachment 162956 [details] [diff] [review]
check the ptr before PR_Open

r=wtc.

Comment 9

[Julien Pierre]

checked in to NSS_3_9_BRANCH
Checking in cmsutil.c;
/cvsroot/mozilla/security/nss/cmd/smimetools/cmsutil.c,v  <--  cmsutil.c
new revision: 1.48.4.1; previous revision: 1.48
done

and the tip
Checking in cmsutil.c;
/cvsroot/mozilla/security/nss/cmd/smimetools/cmsutil.c,v  <--  cmsutil.c
new revision: 1.52; previous revision: 1.51
done