Created attachment 143528 [details] remove "[ foo ] && bar" hack Rc scripts (/etc/rc.d/*) is written to return status returned by the last executed command. This is correct behavior, but it cause problems with construction like this: [ -n "${foo}" ] && echo '.' This construction may return 1 and if it is the last command, whole script return 1. This behavior was certainly not intended, I guess. So construction above should be changed into correcrt form: if [ -n "${foo}" ]; then echo '.' fi I find this bug in /etc/rc.d/routing, but more scripts are affected. Situation when bug appear depends on configuration of services. Patch removing this hack from all scripts is attached.
While here, we could remove some of the x$foo style nonsense, as this hasn't been a problem for decades. I'll have a look later.
Sorry, not going to have time for this :/
(In reply to Chris Rees from comment #2) If I can help, I am ready to do it.
Hey, Thanks. You'll need to look in etc/rc.d find any occurrences of [ something ] && something and append || true to the end. If you provide a patch I'll help to nag people to get it in (I don't have a sec bit). Chris
Chris, there has been a patch attached since 2014-06-08 14:35:23 UTC. I have sent a request to have Devin Teske give this a quick look to determine what the best way to fix this would be. Depending on that feed back I plan to take action on this bug.
The attached patch upon review should NOT be applied. There is a better way that produces smaller diff while maintaining logical operands. The principal reason why the following can produce error status is because the wrong logical operand is used with the wrong condition: [ -n "${foo}" ] && echo '.' We will call the following the "l-value" to the logical AND operator (&&): [ -n "${foo}" ] and we will call the following the "r-value" to '&&' operator: echo '.' The r-value is only evaluated when the l-value is true, and thus when the l-value returns false the logical AND result is likewise false (error status). If you flip the conditional on its head and use the opposite logical operator (the OR operator in this case; "||"), then the l-value leaves a desirable success-status and desirably the r-value is not executed when the inverted condition is true. That sounds like a headache, but it's really simple. The above translates to: [ -z "${foo}" ] || echo '.' Here we say that if ${foo} expands to an empty string (unset or NULL), we don't execute the r-value. The net-effect is that "||" is almost always what you want when you are concerned about exit status. However, you have to be very careful in the rc.d world when you flip an "l-value && r-value" into an "! l-value || r-value" because of the possibility that "set -e" may become neutered for a particular condition. The "set -e" directive in shell makes all errors fatal. The below will trigger a premature termination when "set -e" is in-effect (because the logical AND is only successful if both l-value and r-value return success, a failure by either l-value or r-value will result in premature termination): [ -n "${foo}" ] && echo '.' Meanwhile, flipping this on it's head, despite appearing to not have an effect code-wise, would negate a premature termination should "set -e" be in-effect: [ -z "${foo}" ] || echo '.' That's because the result of a logical OR between two commands is going to be success if either the l-value or r-value returns successfully. Therefore, the way to address the reported problem of unassailable return status is not to blindly translate all logical AND operations into if-statements as the original patch would have done, nor do we blindly translate to logical OR operations, but really we need to look at each use-case of logical AND and/or OR and determine: a. Should premature termination occur for either l-value or r-value in the event of a "set -e" b. Is the logical operation at the end of a function or script If neither of those things are true, leave it alone if it accommodates (a) above correctly as-intended.
And Devin neatly explains why I didn't take the patch :) Perhaps I should have been more explicit!
Chris, If you see a patch submitted on a bugzilla that you believe is incorrect you should state so clearly in a way that there is no doubt that it should not be used, otherwise we run the risk of bad patches submitted ending up in tree.
(In reply to Devin Teske from comment #6) It is true that 'set -e' may be a factor with this kind of changes, but there are two factors which make it almost irrelevant here: * The functions in rc.subr are not supported with 'set -e' in effect. This applies both while sourcing /etc/rc.subr and while calling functions defined in it. * 'set -e' only causes an immediate exit when an untested simple command, pipeline or subshell returns a non-zero exit status. One of the contexts considered "tested" is any command before && or ||. Therefore, a compound command false && true will not cause an immediate exit by itself, but only if its non-zero exit status propagates to an outer function call, pipeline or subshell which is itself untested.