Bug 190793 - Some rc scripts return non zero status on success
Summary: Some rc scripts return non zero status on success
Status: Open
Alias: None
Product: Base System
Classification: Unclassified
Component: conf (show other bugs)
Version: 10.0-STABLE
Hardware: Any Any
: --- Affects Some People
Assignee: freebsd-rc mailing list
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2014-06-08 14:35 UTC by belzebubc
Modified: 2018-02-03 14:23 UTC (History)
6 users (show)

See Also:


Attachments
remove "[ foo ] && bar" hack (7.64 KB, text/plain)
2014-06-08 14:35 UTC, belzebubc
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description belzebubc 2014-06-08 14:35:23 UTC
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.
Comment 1 Chris Rees freebsd_committer 2017-08-31 07:45:23 UTC
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.
Comment 2 Chris Rees freebsd_committer 2018-01-12 12:22:45 UTC
Sorry, not going to have time for this :/
Comment 3 yvesguerin 2018-01-12 18:24:01 UTC
(In reply to Chris Rees from comment #2)
If I can help, I am ready to do it.
Comment 4 Chris Rees freebsd_committer 2018-01-12 21:55:25 UTC
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
Comment 5 Rodney W. Grimes freebsd_committer 2018-01-13 04:32:19 UTC
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.
Comment 6 Devin Teske freebsd_committer 2018-01-13 04:53:42 UTC
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.
Comment 7 Chris Rees freebsd_committer 2018-01-13 07:54:10 UTC
And Devin neatly explains why I didn't take the patch :)  Perhaps I should have been more explicit!
Comment 8 Rodney W. Grimes freebsd_committer 2018-01-13 15:43:05 UTC
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.
Comment 9 Jilles Tjoelker freebsd_committer 2018-02-03 14:23:38 UTC
(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.