Last Comment Bug 565296 - shlibsign returns 0 although it fails
: shlibsign returns 0 although it fails
Status: RESOLVED FIXED
FIPS
:
Product: NSS
Classification: Components
Component: Tools (show other bugs)
: 3.12.4
: All Linux
: -- normal (vote)
: 3.15.1
Assigned To: Kai Engert (:kaie)
:
Mentors:
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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


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

Description Ales Marecek 2010-05-12 06:54:20 PDT
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) 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 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.

bob
Comment 3 Robert Relyea 2010-05-13 11:02:33 PDT
Clearly no longer unconfirmed.
Comment 4 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 Robert Relyea 2013-06-07 16:06:01 PDT
Comment on attachment 742571 [details] [diff] [review]
patch v1

r+ rrelyea
Comment 6 Kai Engert (:kaie) 2013-06-11 12:15:27 PDT
https://hg.mozilla.org/projects/nss/rev/d23a241f9ab5

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