Last Comment Bug 565296 - shlibsign returns 0 although it fails
: shlibsign returns 0 although it fails
Product: NSS
Classification: Components
Component: Tools (show other bugs)
: 3.12.4
: All Linux
-- normal (vote)
: 3.15.1
Assigned To: Kai Engert (:kaie)
Depends on:
Blocks: FIPS2010
  Show dependency treegraph
Reported: 2010-05-12 06:54 PDT by Ales Marecek
Modified: 2013-06-11 12:15 PDT (History)
1 user (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

patch v1 (1.35 KB, patch)
2013-04-26 14:56 PDT, Kai Engert (:kaie)
rrelyea: review+
Details | Diff | Splinter Review

Description User image Ales Marecek 2010-05-12 06:54:20 PDT
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv: 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; echo "
---> returned: $?"
Actual Results:  
moduleSpec configdir='' certPrefix='' keyPrefix='' secmod='' flags=noCertDB,
Generate a DSA key pair ... -5950: File not found
 ---> returned: 0

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

It is not important what to return but it should be non-zero value.
Comment 1 User image Nelson Bolyard (seldom reads bugmail) 2010-05-12 22:03:14 PDT
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 User image Robert Relyea 2010-05-13 11:02:16 PDT
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.

Comment 3 User image Robert Relyea 2010-05-13 11:02:33 PDT
Clearly no longer unconfirmed.
Comment 4 User image Kai Engert (:kaie) 2013-04-26 14:56:22 PDT
Created attachment 742571 [details] [diff] [review]
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 User image Robert Relyea 2013-06-07 16:06:01 PDT
Comment on attachment 742571 [details] [diff] [review]
patch v1

r+ rrelyea
Comment 6 User image Kai Engert (:kaie) 2013-06-11 12:15:27 PDT

Note You need to log in before you can comment on or make changes to this bug.