565296 - shlibsign returns 0 although it fails

Status: RESOLVED FIXED

(NSS :: Tools, defect)

Description

[Ales Marecek]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.3) Gecko/20100412 Red Hat/3.6.3-4.el6 Firefox/3.6.3
Build Identifier: 

Description of problem:
shlibsign returns zero value although it fails.

Reproducible: Always

Steps to Reproduce:
1. /usr/lib/nss/unsupported-tools/shlibsign -v -i nonexistent_lib.so; echo "
---> returned: $?"
Actual Results:  
moduleSpec configdir='' certPrefix='' keyPrefix='' secmod='' flags=noCertDB,
noModDB
Generate a DSA key pair ... 
nonexistent_lib.so: -5950: File not found
 ---> returned: 0

Expected Results:  
moduleSpec configdir='' certPrefix='' keyPrefix='' secmod='' flags=noCertDB,
noModDB
Generate a DSA key pair ... 
nonexistent_lib.so: -5950: File not found
 ---> returned: 1

It is not important what to return but it should be non-zero value.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Having re-read the code, it's apparent to me that the author(s) didn't 
intend for it to return non-zero except in the case where the attempt to 
shutdown the PKCS#11 module used for signing failed.  So, this is a case 
of mismatched expectations.  

Now, I cannot explain or defend the decision to return non-zero solely in
that case.  I too would expect it to return non-zero on any failure.  
But it's always done what it is now doing, and I am concerned that changing
it now could break scripts and/or builds.  

Bob, what do you think?

Comment 2

[Robert Relyea]

I think the change is fine. shlibsign is outside the FIPS boundary, so it's OK to change without a revalidation.

I think I would want to know what scripts and builds fail if we make this change. It sounds like a bug in the script or build. I believe the owners of those are either the NSS team, or close enough to the NSS team that they can be fixed if there is a problem.

bob

Comment 3

[Robert Relyea]

Clearly no longer unconfirmed.

Comment 4

[Kai Engert [:KaiE:]]

Attachment: patch v1

The statement in comment 1 is incorrect - the function returns the non-zero error return code in many scenarios:
- if incorrect arguments are given
- whenever a call to a pkcs#11 API fails

However, for any non-pkcs#11 layer related failures, the function will return zero, and in the success scenario, it will return zero, too.

I believe it makes absolute sense to return non-zero in any failure scenario.

I cannot imagine that any tests break if we make that change.
If it does, those would rather have been undetected failures!

I'm attaching a trivial patch that implements the request.
I've ran the test suite, and it still passes.

Comment 5

[Robert Relyea]

Comment on attachment 742571 [details] [diff] [review]
patch v1

r+ rrelyea

Comment 6

[Kai Engert [:KaiE:]]

https://hg.mozilla.org/projects/nss/rev/d23a241f9ab5