451926 - MSYS shell extracts a negative exit status as 0.

Status: RESOLVED INACTIVE

(NSS :: Test, defect, P5)

Description

[Wan-Teh Chang]

Attachment: Test program foo.c that returns its argument as exit status

To be portable, a program's exit status should be 0-255.
See http://www.opengroup.org/onlinepubs/000095399/functions/exit.html

If a program exits with the status code -1, it is usually
reported to the parent process as 255.  But this is not
portable.  I remember MKS shell has problem with this.
Tonight, I found that MSYS shell also can't handle this.
But its failure mode is worse: -1 is reported to the parent
process as 0, which means success!

I first verified this by changing the statement at the end
of the main function in cmd/signtool/signtool.c:
    return retval;
to
    return -1;
and found that the tools.sh script reported PASS on all the
signtool tests.

Attached is a simple test program foo.c that returns its
command-line argument as exit status.  I will next attach
a shell script that performs experiments using foo.c.

Comment 1

[Wan-Teh Chang]

Attachment: Shell script for running test program 'foo'

This shell script prints the exit codes it receives from
test programs that exit with 258,257,256,255,254,
2,1,0,-1,-2.

On Mac OS X, the output is:
2
1
0
255
254
2
1
0
255
254

On Windows under MSYS, the output is:
2
1
0
255
254
2
1
0
0
0

Comment 2

[Wan-Teh Chang]

On Windows under MKS, the output is:
130
[1] + Done(129) ?
  484   Hangup  ./foo
129
1
255
254
2
1
0
[1] + Done(134) ?
  4448  Abort   ./foo
134
[1] + Done(134) ?
  8068  Abort   ./foo
134

Note the error messages for 257,-1,-2, and the non-zero
exit codes for 258,257,256,-1,-2.

On Windows under Cygwin, the output is:
2
1
0
255
254
2
1
0
255
254

Comment 3

[Wan-Teh Chang]

Attachment: Patch for signtool (exit with 255 instead of -1)

signtool exits with -1 on failure right now.
Ideally it should exit with 1 on failure.
Just in case someone depends on the exit codes
used by signtool, I am using 255 in this patch.
As my experiments showed, a working shell should
report an exit code of -1 as 255.

Comment 4

[Wan-Teh Chang]

Slavo, do you know which NSS commands are executed
by all.sh?  I'll need to review all of them to make
sure they don't exit with a negative exit code.

By searching for ${BINDIR}/xxx under the
mozilla/security/nss/tests directory, I found
these:

certutil
crlutil
modutil
bltest
crmftest
dbtest
cmsutil
pk12util
signtool
pk11mode
mangle
selfserv
tstclnt
strsclnt
ocspclnt
sdrtest
rsaperf
vfychain
p7env
p7content
p7sign
p7verify

Is this the right method to find out which NSS
commands are run by all.sh?

Can you help me review these commands to make sure
they don't exit with a negative exit code?  For
each command, you need to check two things:

1. The main function doesn't return a negative
number.

2. Search for "exit" in all files.  Negative numbers
should not be passed to exit().

Comment 5

[Wan-Teh Chang]

Attachment: Patch for signtool (exit with 255 instead of -1) v2

I missed an exit(-1) call in cmd/signtool/util.c.

Comment 6

[Wan-Teh Chang]

The MSYS upstream bug report is
https://sourceforge.net/tracker/index.php?func=detail&aid=1172090&group_id=2435&atid=102435

Comment 7

[Slavomir Katuscak]

(In reply to comment #4)
> Is this the right method to find out which NSS
> commands are run by all.sh?

Some months ago I modified testing scripts to use ${BINDIR} for all NSS binaries. I hope this rule is respected in new patches, but I haven't reviewed them all. 

> Can you help me review these commands to make sure
> they don't exit with a negative exit code?  For
> each command, you need to check two things:
> 
> 1. The main function doesn't return a negative
> number.
> 
> 2. Search for "exit" in all files.  Negative numbers
> should not be passed to exit().

I'll review them and let you now.

Comment 8

[Wan-Teh Chang]

Slavo, last Friday night I spent some time reviewing
the tools I listed in comment 4.

I only had time to review certutil, crlutil, modutil,
bltest.  So you don't need to review them again.

I found exit(-1) or main function returning -1 in
each of them.

Comment 9

[Slavomir Katuscak]

Wan-Teh, I looked over some of those sources, and it seems that we have there always some of situations:

1. exit(-1) -> can be moved to 255
2. return rv -> rv is collected from many other functions, doesn't make much sense to go deeper into it, rather use construction like return rv >= 0 ? rv : 255
3. return SECSuccess/SECFailure -> use numeric values 

Reviewing the code for those cases and fixing it is redundant job, it can be done at once.