To button this issue up, it's clear to me that Jim had transposed the meaning of result values from posix commands, and that was the origin of irrationality in this discussion.
Beyond the misunderstanding, the actual behavior of openssl in 1.0.x and prior was inane, and led to Jim's confusion, and my earlier hint to add -help would not have improved the situation. On Sun, Oct 14, 2018 at 3:50 PM Rainer Jung <[email protected]> wrote: > Am 14.10.2018 um 21:59 schrieb William A Rowe Jr: > > On Sun, Oct 14, 2018 at 8:32 AM Jim Jagielski <[email protected] > > <mailto:[email protected]>> wrote: > > > > All we are checking is the error code. Nothing else. > > > > % openssl version > > OpenSSL 1.0.2p 14 Aug 2018 > The result of this command, $? had returns a value 0, success. > > % openssl ocsp 2>/dev/null > > % print $? > > 1 > This result code is failure. A non-zero posix result code is failure. See `man 3 system`. (See also perldoc system, search for LIST to learn that the result code is shifted 8 bits, returning 256 for '1'.) > % openssl foo 2>/dev/null > > % print $? > > 0 > This result code is success, return code 0, no error. An unrecognized verb was not an error!?! > > With 1.1.1, both return 1, but so what, we know that it has oscp. > With 1.1.0 and 1.1.1 the failure result code 1 is correct, as shown below. > I can confirm this behavior for normal OpenSSL 1.0.2p. > I also confirm, and my apologies for presuming this was some fork's behavior. OpenSSL 1.0.x behavior was truly incomprehensible as shown below... And worse, my suggested fix to jim was also erroneous, in 1.0.2 and prior the ocsp -help flag is similarly treated as an error. Using 1.0.2 branch (frozen/final) from git; $ openssl version OpenSSL 1.0.2l-dev xx XXX xxxx $ echo $? 0 $ openssl foo openssl:Error: 'foo' is an invalid command. Standard commands asn1parse ca ciphers cms [...] $ echo $? 0 $ openssl ocsp OCSP utility Usage ocsp [options] where options are -out file output filename [...] $ echo $? 1 $ openssl ocsp -help OCSP utility Usage ocsp [options] where options are -out file output filename [...] $ echo $? 1 Success, success, failure, failure. Which makes no fricking sense, and this was the case jim sought to solve for. Once we are at 1.1.0, this all starts to right itself; $ openssl version OpenSSL 1.1.0i-fips 14 Aug 2018 $ echo $? 0 $ openssl foo Invalid command 'foo'; type "help" for a list. $ echo $? 1 $ openssl ocsp 2>&1 ocsp: Use -help for summary. $ echo $? 1 $ openssl ocsp -help 2>&1 Usage: ocsp [options] Valid options are: -help Display this summary -out outfile Output filename [...] $ echo $? 0 Success, failure, failure, success, as would be expected. It's plainly obvious that the result code from openssl main() cannot be trusted for evaluating features between 1.0.x and 1.1.x, and is the reason Jim's patch was defective. So we are back to testing the output of `openssl list -commands 2>&1`, evaluating stdout on 1.1.x, and stderr on 1.0.x. Unless Jim's veto stands? > On Mon, Oct 15, 2018 at 10:07 AM Jim Jagielski <[email protected]> wrote: >> >> -1 (veto) >> >> Please revert. 'list' is NOT a command and this causes OCSP to be skipped. On Mon, Oct 15, 2018 at 10:20 AM Jim Jagielski <[email protected]> wrote: > > Forget this. My patch works and is correct and handles the specific situation which is noted in the test case itself related to older versions. It is an IMPROVEMENT over what we currently have. Your patch treated success as failure and failure as success. Your patch had enabled ocsp always in 1.1.x because `openssl ocsp` always returns failure. It was disabling the test when absent from 1.0.x because `openssl ocsp` returned failure, while it enabled the test with 1.0.x because `openssl {unknown}` returned success when unrecognized. Your patch introduced a regression because ocsp in 1.1.x must not be tested when configured no-ocsp; this was harmful. My suggestion of `openssl ocsp -help` would not have fixed this, because then the test would have been disabled in 1.1.x when ocsp was available, and enabled when ocsp was absent. However, using a valid command would have helped identify the underlying logic error. Joe's original code enabled ocsp correctly in 1.1.x because `openssl list -commands` works, and the /ocsp/ string is found. Joe's original code never enabled the test in 1.0.2, which was mostly harmless, but less effective. My patch to Joe's original code works because `openssl list -commands` fails so the command list is emitted to stderr, where we now detect /ocsp/. > The sole reason why Bill doesn't like it is because *I* committed it. By now, we should agree that this tantrum was uncalled for, in light of these multiple defective patches. The fact that you couldn't believe your logic might be erroneous wasn't an excuse to have a meltdown or take a code discussion personally. > Whatever. I have no desire or patience with him anymore. I could not care less what patch is included. You sent this 13 minutes after your veto, so I will read this as withdrawing your veto above. If I'm misunderstanding, please state your veto again to avoid confusion and I will revert to jorton's logic, removing my fix for 1.0.x. Please stop assuming ill will when your defects are pointed out. Your misunderstanding of system() is not my problem. My issues were with the 'try it until something looks like it works' approach of your patches, and not you as a person.
