Bug 638955 - Incorrect value of $status ($?) in case of pipelines
Summary: Incorrect value of $status ($?) in case of pipelines
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat Enterprise Linux 5
Classification: Red Hat
Component: tcsh617
Version: 5.6
Hardware: All
OS: Linux
high
medium
Target Milestone: rc
: ---
Assignee: Vojtech Vitek
QA Contact: BaseOS QE - Apps
URL:
Whiteboard:
Depends On:
Blocks: 590060 658190 759132 784510
TreeView+ depends on / blocked
 
Reported: 2010-09-30 13:34 UTC by Tomas Smetana
Modified: 2018-11-30 22:46 UTC (History)
11 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
This package fixes the return value of the "status" (or "$?") variable in the case of pipelines and backquoted commands. The "anyerror" variable, which selects behavior, has been added to retain the backward compatibility.
Clone Of:
: 658190 759132 (view as bug list)
Environment:
Last Closed: 2011-07-21 08:48:23 UTC
Target Upstream Version:
Embargoed:


Attachments (Terms of Use)
Proposed patch (529 bytes, patch)
2010-09-30 13:36 UTC, Tomas Smetana
no flags Details | Diff
Second attempt (557 bytes, patch)
2010-10-15 11:48 UTC, Tomas Smetana
no flags Details | Diff
New patch (1.28 KB, patch)
2011-02-03 14:20 UTC, Tomas Smetana
no flags Details | Diff
Patch to fix $status of pipelined/backquoted commands (1.54 KB, patch)
2011-04-14 09:58 UTC, Vojtech Vitek
no flags Details | Diff
Patch to fix the testsuite that expected wrong behaviour (1.23 KB, patch)
2011-04-14 09:59 UTC, Vojtech Vitek
no flags Details | Diff
Upstream accepted patch with additional $anyerror variable to select behavior (6.02 KB, patch)
2011-04-15 11:56 UTC, Vojtech Vitek
no flags Details | Diff


Links
System ID Private Priority Status Summary Last Updated
Red Hat Bugzilla 759132 0 high CLOSED Fix regression in default $status value in case of lists/pipelines 2021-02-22 00:41:40 UTC
Red Hat Knowledge Base (Legacy) 40784 0 None None None Never
Red Hat Product Errata RHBA-2011:1072 0 normal SHIPPED_LIVE new package: tcsh617 2011-07-21 08:47:46 UTC

Internal Links: 735902 759132

Description Tomas Smetana 2010-09-30 13:34:30 UTC
Description of problem:
The tcsh documentation states that the $status (or $?) variables hold the return value of the last command. Therefore in case of pipelines it should be the last command of the pipeline. This doesn't seem to be working OK.

Version-Release number of selected component (if applicable):
tcsh-6.14-18.el5

How reproducible:
Always

Steps to Reproduce:

1. cat no-exist | wc -l
2. echo $status (or echo $?)
  
Actual results:
1

Expected results:
0

Additional info:
The $? variable should hold the return code of "wc -l" which is 0. Similarly for the backquoted pipeline: echo `cat no-exist | wc -l` ($status would be 1, expected value is 0).

Comment 1 Tomas Smetana 2010-09-30 13:36:22 UTC
Created attachment 450757 [details]
Proposed patch

This fixes the problem in the description in my environment. Not tested very thoroughly yet.

The patch should cause tcsh to ignore exit value of processes with output redirected to a pipe.

Comment 3 Tomas Smetana 2010-10-15 11:48:11 UTC
Created attachment 453711 [details]
Second attempt

The previous patch didn't fix the case of
    echo `cat nonexistant`

Expected return value of this expression is 0 ('echo' succeeded) however tcsh returned 1.

The attached patch is supposed to fix that -- few first tests look OK. Comments welcome.

Comment 4 Shinji Kito 2010-10-18 05:24:36 UTC
Our partner also verified the test package.

<snip>
The test package worked fine. 
</snip>

Comment 5 Vojtech Vitek 2010-11-29 15:57:16 UTC
Have you tried to propose this patch to upstream?

Comment 6 Tomas Smetana 2010-11-30 09:20:27 UTC
Hello,

I haven't send the fix upstream yet.  I will try to find a place where to file a bug. To be honest -- if that would require me to subscribe to another mailing list, I'd rather leave it up to you...

Regards.

Comment 7 Vojtech Vitek 2010-12-02 11:40:02 UTC
I have submitted patch to upstream for you:
http://bugs.gw.com/view.php?id=110

Comment 8 Tomas Smetana 2010-12-02 11:52:15 UTC
OK. Thank you.

Comment 9 Vojtech Vitek 2011-01-18 10:38:01 UTC
The patch has been accepted by upstream (6.17.03b).

Comment 11 Vojtech Vitek 2011-01-19 10:21:03 UTC
Upstream reverted the patch: "Undo PR/110 breaks unit tests."

Comment 12 Vojtech Vitek 2011-02-01 12:11:02 UTC
The upstream tcsh 6.17.04 test suite with the _reverted_ patch applied:

110: Primary expressions                             FAILED (expr.at:84)

AT_CHECK([tcsh -f -c 'exit { false }'], 0)

113: Escaping special characters                     FAILED (lexical.at:60)

AT_DATA([nosplit.csh],
[[echo \&\|\;\<\>\(\)\&\|\|\<\<\>\>\space\ tab\ end
echo '&|;<>()&||<<>>space tab   end'
echo "&|;<>()&||<<>>space tab   end"
set verbose
echo `&|;<>()&||<<>>space tab   end`
]])
AT_CHECK([tcsh -f nosplit.csh], 1,
[&|;<>()&||<<>>space tab        end
&|;<>()&||<<>>space tab end
&|;<>()&||<<>>space tab end

],
[echo `&|;<>()&||<<>>space tab  end`
Invalid null command.
])

114: Preventing substitution                         FAILED (lexical.at:135)

AT_CHECK([tcsh -f -c 'echo "`OK`"'], 1, [
],
[OK: Command not found.
])

123: Command substitution                            FAILED (subst.at:17)

AT_DATA([backq.csh],
[[set a=(a`echo 1 2; echo 3 4`b)
echo $#a
set a=(a"`echo 1 2; echo 3 4`b")
echo $#a
unset csubstnonl
echo `echo 1; \\
echo 2`
set csubstnonl
echo `echo 1; \\
  echo 2`
]])
AT_CHECK([tcsh -f backq.csh], 1,
[4
2
1 2
1
],
[ : Command not found.
])

127: Command execution                               FAILED (syntax.at:159)

AT_CHECK([tcsh -f -c '(echo $this_does_not_exist) |& cat'], 1,
[this_does_not_exist: Undefined variable.
])

Comment 13 Tomas Smetana 2011-02-01 13:34:13 UTC
If I understand what the test macros actually do, then the tests actually expect a wrong behaviour.  No wonder they broke...

Comment 14 Vojtech Vitek 2011-02-02 10:31:11 UTC
The only failed test shows us a bug we should figure out:

110: Primary expressions                             FAILED (expr.at:84)
AT_CHECK([tcsh -f -c 'exit { true }'], 1)
AT_CHECK([tcsh -f -c 'exit { false }'], 0)

$ ./tcsh
$ exit { true }
exit
$ echo $?
1

$ ./tcsh
$ exit { false }
exit
$ echo $?
1

Manual page tcsh(1) line 1736:
  exit [expr]
    The shell exits either with the value of the specified  expr
    (an  expression,  as  described  under Expressions) or, without expr,
    with the value 0.

Manual page tcsh(1) line 1085
  Command exit status
    Commands  can  be executed in expressions and their exit status returned
    by enclosing them in braces (`{}').
    Remember that the braces should be separated from the words of the command
    by  spaces.   Command  executions succeed,  returning  true,  i.e.,  `1',
    if  the command exits with status 0, otherwise they fail, returning
    false, i.e., `0'.  If more detailed status information is required then
    the command should be executed  out‐side of an expression and the status
    shell variable examined.

Comment 15 Tomas Smetana 2011-02-02 11:58:09 UTC
OK. I'll try to take a look at the patch again.

Thanks.

Comment 16 Tomas Smetana 2011-02-03 14:20:42 UTC
Created attachment 476793 [details]
New patch

Looks that tcsh treats {} and `` constructions similarly though their semantic is a bit different.  Here's another patch which should fix the problem in the description and not regress on the 'exit { false }' test.

I aimed for a minimal change so the patch is quite ugly: I set the process flag by the actual command string while allocating the process structure, which doesn't seem "right" -- IMHO it belongs to the parser... Feel free to rewrite -- I think where the problem lies is clear from the attached patch.

Comment 21 Vojtech Vitek 2011-04-14 09:58:11 UTC
Created attachment 492015 [details]
Patch to fix $status of pipelined/backquoted commands

This is slightly modified patch by Tomas Smetana (Comment 16) that fixes possible segmentation fault.

Comment 22 Vojtech Vitek 2011-04-14 09:59:14 UTC
Created attachment 492016 [details]
Patch to fix the testsuite that expected wrong behaviour

Comment 23 Vojtech Vitek 2011-04-15 11:00:37 UTC
The new patch has been accepted by upstream (6.17.06b).

Comment 24 Vojtech Vitek 2011-04-15 11:56:16 UTC
Created attachment 492357 [details]
Upstream accepted patch with additional $anyerror variable to select behavior

Comment 27 Branislav Náter 2011-05-13 12:42:44 UTC
Bugfix was successfully verified on package tcsh617-6.17-4.el5 on all supported
architecutres (i386, x86_64, s390x, ppc64 and ia64).

$status (or $?) now contain return value of last command.

Comment 28 Miroslav Svoboda 2011-07-01 21:53:06 UTC
    Technical note added. If any revisions are required, please edit the "Technical Notes" field
    accordingly. All revisions will be proofread by the Engineering Content Services team.
    
    New Contents:
This package fixes the return value of the "status" (or "$?") variable in the case of pipelines and backquoted commands. The "anyerror" variable, which selects behavior, has been added to retain the backward compatibility.

Comment 29 errata-xmlrpc 2011-07-21 08:48:23 UTC
An advisory has been issued which should help the problem
described in this bug report. This report is therefore being
closed with a resolution of ERRATA. For more information
on therefore solution and/or where to find the updated files,
please follow the link below. You may reopen this bug report
if the solution does not work for you.

http://rhn.redhat.com/errata/RHBA-2011-1072.html

Comment 30 errata-xmlrpc 2011-07-21 12:09:21 UTC
An advisory has been issued which should help the problem
described in this bug report. This report is therefore being
closed with a resolution of ERRATA. For more information
on therefore solution and/or where to find the updated files,
please follow the link below. You may reopen this bug report
if the solution does not work for you.

http://rhn.redhat.com/errata/RHBA-2011-1072.html

Comment 31 Angelo Bonet 2011-11-17 17:13:19 UTC
So let me get this straight...

csh/tcsh has been setting $status from pipelines and backquotes the same way for ~20 years now (going back to pure-csh on SunOS and including tcsh on RHEL 3,4,5, and 6)?  But there's been a disconnect between the behavior and what's documented in the man page?

So the solution is to give preference to the documentation? 

My entire enterprise is dependent on a piece of software that makes heavy use of csh/tcsh -- and for the past 20 years our customers have built more csh/tcsh infrastructure on top of it.  tcsh617 (with this change) has already broken some of our scripts -- and I've identified more places where it possibly could.

So now I'm supposed to accept the fact that my enterprise software's legacy of testing is negated if I a) install tcsh617, b) migrate to RHEL5.7 (which runs tcsh617) or c) migrate to RHEL 6.2 (which will probably ship with tcsh-6.17-13.el6:  https://bugzilla.redhat.com/show_bug.cgi?id=658190)?

And because I'm being affected by this bug #618723 (https://bugzilla.redhat.com/show_bug.cgi?id=618723) in RHEL5 *and* it's only been fixed in tcsh617 (and for some reason not backported into tcsh-6.14), I'm now forced to choose between two "solutions" that each break different parts of my software.

The anyerror shell variable is a nice thought, but since it's a shell variable it won't propagate to subshells -- which makes it hard to globally affect "#!/bin/csh -f" scripts.

IMO, this entire change should be completely redacted.  This should have been considered a documentation bug and the manpage reworded to reflect the way the thing's been working forever.

If you're still hellbent on introducing this behavior, then introduce a "lasterror" shell variable that I can set to **opt-in** to the new behavior, instead of opting-in to the old behavior.

Comment 32 Vojtech Vitek 2011-11-22 16:23:11 UTC
(In reply to comment #31)
> IMO, this entire change should be completely redacted.  This should have been
> considered a documentation bug and the manpage reworded to reflect the way the
> thing's been working forever.
The thing is, that the correct behaviour is defined by POSIX (see http://pubs.opengroup.org/onlinepubs/000095399/utilities/xcu_chap02.html#tag_02_09_02 ). So, it's not just a 'random mistake' in man page.

Our primary target was to fix the wrong behaviour and prevent compatibility issues with other POSIX-compliant shells (bash, ksh, resh etc.).

Upstream tcsh developer, who maintains tcsh since 1987, has also accepted the changes (see http://bugs.gw.com/view.php?id=110 ), making this default behaviour starting with TCSH version 6.17.05.

> If you're still hellbent on introducing this behavior, then introduce a
> "lasterror" shell variable that I can set to **opt-in** to the new behavior,
> instead of opting-in to the old behavior.
However, I understand your concerns and I'm willing to discuss possible changes to fix your issue (reverting the changes / making the old behaviour default).

But please, beware that Bugzilla is not a customer support tool. I recommend you contacting your support representative to escalate this request.
[ Global Support Services, http://www.redhat.com/support/ ]

Comment 33 Angelo Bonet 2011-11-22 18:11:27 UTC
(In reply to comment #32)

> The thing is, that the correct behaviour is defined by POSIX (see
> http://pubs.opengroup.org/onlinepubs/000095399/utilities/xcu_chap02.html#tag_02_09_02
> ). So, it's not just a 'random mistake' in man page.

If you read that document, you'll realize it has absolutely nothing to do with csh/tcsh -- which are not part of the POSIX shell family. There are literally dozens of things in that document that are not compatible with csh/tcsh.


> Our primary target was to fix the wrong behavior

The "wrong" behavior has been that way for 20+ years -- which (for better or for worse) makes it the "right" behavior at this point.

> and prevent compatibility issues with other POSIX-compliant shells (bash, ksh, resh etc.).

Then your entire premise for this change was misguided -- another reason to redact it.  Once again, POSIX-compliant shells and csh/tcsh are two completely different things.  csh/tcsh will *never* be POSIX compliant -- they're not supposed to be compatible.


> Upstream tcsh developer, who maintains tcsh since 1987, has also accepted the
> changes (see http://bugs.gw.com/view.php?id=110 ), making this default
> behaviour starting with TCSH version 6.17.05.

I saw that and I'm surprised he actually accepted it.  Remember, he hasn't actually *released* a version of tcsh with this change in it -- and once he realizes the impact, he may not release it.  At this point, no one in the world has a tcsh in the wild that behaves this way except for Redhat.  


> However, I understand your concerns and I'm willing to discuss possible changes
> to fix your issue (reverting the changes / making the old behaviour default).

Here's the situation: csh/tcsh scripts that have been running for 20+ years on various platforms (including RHEL) will now potentially break if they're run on your new tcsh implementation.   Your new tcsh is now incompatible with every other tcsh implementation.  Plain and simple!

In addition, the change you made was completely gratuitous -- there was no demand for it -- customer or otherwise.

RHEL is touted as enterprise software -- your engineers can't go and start changing long-standing, well-established behaviors/APIs to match quirks in documentation.  Maintaining stability within a release series should be paramount. 

Like I've proposed, the minimum you should do is make the new behavior opt-in (instead of opt-out).  But even that is risky (e.g. a legacy script might happen to use the same shell variable name that activates this new behavior -- and their script runs different).


> But please, beware that Bugzilla is not a customer support tool. I recommend
> you contacting your support representative to escalate this request.
> [ Global Support Services, http://www.redhat.com/support/ ]

We have already opened a formal support request.

Comment 35 Angelo Bonet 2011-11-29 19:21:40 UTC
For the record, we're also trying to let upstream know that this was a bad idea:

http://bugs.gw.com/view.php?id=155

Comment 36 Vojtech Vitek 2011-11-30 13:46:00 UTC
(In reply to comment #35)
> For the record, we're also trying to let upstream know that this was a bad
> idea:
> 
> http://bugs.gw.com/view.php?id=155

Thanks Angelo.

I'm going to write a backward-compatible patch, that would rather opt-in the "new" behaviour.

Comment 37 Angelo Bonet 2011-11-30 21:23:26 UTC
(In reply to comment #36)
> (In reply to comment #35)
> > For the record, we're also trying to let upstream know that this was a bad
> > idea:
> > 
> > http://bugs.gw.com/view.php?id=155
> 
> Thanks Angelo.
> 
> I'm going to write a backward-compatible patch, that would rather opt-in the
> "new" behaviour.


Thanks for responding to this.  

Making the new behavior opt-in is a much more sound approach.  Although keep in mind that csh/tcsh makes no distinction between "special" shell variables that control the shell's behavior (e.g. noclobber, ignoreeof, noglob) and plain-old local variables used in a script.

So, if someone already has a script where they're using "set" variables named "anyerror" or "lasterror" (or whatever your variable will be named), they could accidentally enable behavior they don't want.


Ideally, I would just want to see tcsh left alone and absent of any "new" behaviors.  Short of that, please choose your variable name judiciously to reduce the risk of collisions.

Comment 40 Vojtech Vitek 2011-12-01 14:07:45 UTC
(In reply to comment #37)
> Thanks for responding to this.  

I've created new bug 759132 to cover the Regression (we can't reopen this bugzilla, as it was already shipped within the ERRATA).

Comment 41 P. A. Cheeseman 2012-03-24 22:57:36 UTC
(In reply to comment #40)
> (In reply to comment #37)
> > Thanks for responding to this.  
> 
> I've created new bug 759132 to cover the Regression (we can't reopen this
> bugzilla, as it was already shipped within the ERRATA).

Does this mean that behavior such as ...

--------
punkin: set r = `ls blotto`
ls: cannot access blotto: No such file or directory
punkin: echo $status
0
punkin: bash
bash-4.1$ r=`ls blotto`
ls: cannot access blotto: No such file or directory
bash-4.1$ echo $?
2
bash-4.1$ exit
exit
punkin: ksh
$ r=`ls blotto`
ls: cannot access blotto: No such file or directory
$ echo $?
2
$ exit
punkin: echo $version
tcsh 6.17.00 (Astron) 2009-07-10 (x86_64-unknown-linux) options wide,nls,dl,al,kan,rh,color,filec
--------

will change such that tcsh will behave as the other shells when the rightmost member of an RHS pipeline returns non-zero exit status?  Or are the other shells going to be changed to match tcsh (the login shell)?

     I've tried to follow the chain of 'issues' on this bug and haven't made much headway so I decided to post here.  Seems to me that the attempt to fix the original report introduced a new bug.

Thanks,
Phil

Comment 42 Vojtech Vitek 2012-03-25 01:39:50 UTC
Hi Phil,

please, take a look at bug 759132 (or bug 784510 for RHEL-6). We are going to revert the changed functionality to tcsh default (as it used to work for many years). There will be new tcsh_posix_status variable available to opt-in POSIX-like behaviour (as in bash, ksh etc.).

Comment 43 Angelo Bonet 2012-03-26 16:52:38 UTC
Phil,

Prior to Redhat changing this behavior in TCSH, it would have returned a non-zero exit status in your example (that's what csh/tcsh has done for decades).

What I find ironic is part of their justification was to "make tcsh behave more like POSIX-shells" (bash, ksh, sh).  But in the examples you posted, they actually made it behave *differently* than POSIX-shells.

IMO, another glaring example of why these changes were so brash and ill-advised.

-Angelo


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