User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:18.104.22.168) 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.
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?
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
Clearly no longer unconfirmed.
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 on attachment 742571 [details] [diff] [review] patch v1 r+ rrelyea