Bug 531353

Summary: tcsh obeys printexitvalue for back-ticks
Product: Red Hat Enterprise Linux 5 Reporter: Jeff Bastian <jbastian>
Component: tcshAssignee: Jaromír Končický <jkoncick>
Status: CLOSED CURRENTRELEASE QA Contact: BaseOS QE - Apps <qe-baseos-apps>
Severity: medium Docs Contact:
Priority: urgent    
Version: 5.4CC: jwest, ovasik, psplicha, rbinkhor, rvokal
Target Milestone: rcKeywords: Patch, Reopened, ZStream
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
: 531957 578817 689381 (view as bug list) Environment:
Last Closed: 2013-09-23 07:05:54 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Bug Depends On:    
Bug Blocks: 502912, 590060, 579251, 640252, 689381    
Attachments:
Description Flags
patch to fix backquoted output none

Description Jeff Bastian 2009-10-27 16:18:03 EDT
Description of problem:
If printexitvalue is set, tcsh returns the exit code as part of the command output, rendering the output unusable.

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

How reproducible:
every time

Steps to Reproduce:
1. set printexitvalue
2. set i = `grep "nonono" /etc/csh.cshrc | wc -l`
3. echo $i
  
Actual results:
0 Exit 1

Expected results:
0

Additional info:
RHEL 4 tcsh-6.13-10.el4 behaves correctly
Comment 1 Jeff Bastian 2009-10-27 16:32:29 EDT
This bug was reported a while ago:
  http://mx.gw.com/pipermail/tcsh-bugs/2006-June/000444.html

However, it's still a bug with the latest upstream release, version 6.17.00 from July 2009.
  ftp://ftp.astron.com/pub/tcsh/tcsh-6.17.00.tar.gz
Comment 2 Jeff Bastian 2009-10-27 17:50:57 EDT
Comparing RHEL 4 and RHEL 5 tcsh code, this chunk is new:

--- RHEL-4/tcsh-6.13.00/sh.sem.c        2009-10-27 15:47:02.906665630 -0500   
+++ RHEL-5/tcsh-6.14.00/sh.sem.c        2009-10-27 15:47:07.391667628 -0500
@@ -656,6 +652,13 @@ execute(t, wanttty, pipein, pipeout, do_                   
            func(t, bifunc);                                                    
            if (forked)                                                         
                exitstat();                                                     
+           else {                                                              
+               if (adrof(STRprintexitvalue)) {                                 
+                   int rv = getn(varval(STRstatus));                           
+                   if (rv != 0)                                                
+                       xprintf(CGETS(17, 2, "Exit %d\n"), rv);                 
+               }                                                               
+           }                                                                   
            break;                                                              
        }                                                                       
        if (t->t_dtyp != NODE_PAREN) {


Now, compare that new chunk to the pjwait() function in sh.proc.c:
    /*
     * Don't report on backquoted jobs, cause it will mess up
     * their output.
     */
    if ((reason != 0) && (adrof(STRprintexitvalue)) &&
        (pp->p_flags & PBACKQ) == 0)
        xprintf(CGETS(17, 2, "Exit %d\n"), reason);


The new chunk above does not check pp->p_flags for the PBACKQ flag; I wonder if it should?
Comment 3 Jeff Bastian 2009-10-27 18:18:43 EDT
Ignore comment 2...  I just tested for backquotes in that chunk but that didn't solve the bug.

diff --git a/sh.sem.c b/sh.sem.c
index 0210e54..7fe828e 100644
--- a/sh.sem.c
+++ b/sh.sem.c
@@ -653,7 +653,8 @@ execute(t, wanttty, pipein, pipeout, do_glob)
            if (forked)
                exitstat();
            else {
-               if (adrof(STRprintexitvalue)) {
+               if ((adrof(STRprintexitvalue)) &&
+                    ((t->t_dflg & F_BACKQ) == 0)) {
                    int rv = getn(varval(STRstatus));
                    if (rv != 0)
                        xprintf(CGETS(17, 2, "Exit %d\n"), rv);
Comment 4 Jeff Bastian 2009-10-27 18:33:39 EDT
Well, maybe there is something to the above chunk after all.  I ran tcsh through gdb and set breakpoints on all the print statements that print an 'Exit %d' type string, and it stopped on the xprintf in the new chunk:


$ rpm -q tcsh tcsh-debuginfo
tcsh-6.14-14.el5.x86_64
tcsh-debuginfo-6.14-14.el5.x86_64

$ cat tcsh-gdb-cmds.txt
b sh.proc.c:640
b sh.proc.c:1160
b sh.sem.c:659
run -f

$ gdb -q -x tcsh-gdb-cmds.txt /bin/tcsh
Breakpoint 1 at 0x41e86a: file sh.proc.c, line 640.
Breakpoint 2 at 0x41d18e: file sh.proc.c, line 1160.
Breakpoint 3 at 0x41fb06: file sh.sem.c, line 659.
> set printexitvalue
> set i=`/bin/false`
Detaching after fork from child process 19285.

Breakpoint 3, execute (t=0xb395930, wanttty=19279, pipein=0x0, pipeout=0x0,
    do_glob=1) at sh.sem.c:659
659                             xprintf(CGETS(17, 2, "Exit %d\n"), rv);
(gdb) print rv
$1 = 1
(gdb) c
Continuing.
Exit 1
> exit

Program exited with code 01.
(gdb)
Comment 5 Jeff Bastian 2009-10-28 14:45:13 EDT
I did some more testing and I see how the two xprintf() statements work with each other now: the new chunk of code in sh.sem.c prints the exit value for built-in commands whereas the chunk in sh.proc.c handles normal commands.

I modified each xprintf() slightly so I could tell the difference:
   1. sh.proc.c
   -       xprintf(CGETS(17, 2, "Exit %d\n"), reason);
   +       xprintf(CGETS(17, 2, "BAR %d\n"), reason);

   2. sh.sem.c 
   -                       xprintf(CGETS(17, 2, "Exit %d\n"), rv);
   +                       xprintf(CGETS(17, 2, "FOO %d\n"), rv);

NOTE: You have to unset the LANG environment variable or the CGETS() call will get the string to print from the i18n files.

Running with this patch
   $ ./tcsh -f
   > unsetenv LANG
   > set printexitvalue
   > /bin/false
   BAR 1
   > set i=`/bin/false`
   FOO 1
   > echo $i
   BAR 1
   > exit

Thus, the bug lies in sh.proc.c since $i should be an empty string.
Comment 6 Jeff Bastian 2009-10-28 15:06:04 EDT
It looks like the PBACKQ flag is not getting set in pp->p_flags; I made the xprintf() a little more verbose:

    if ((reason != 0) && (adrof(STRprintexitvalue)) &&
        (pp->p_flags & PBACKQ) == 0)
        xprintf("Exit %d, pp->p_flags & PBACKQ => 0x%x & 0x%x => 0x%x\n",
                reason, pp->p_flags, PBACKQ, (pp->p_flags & PBACKQ));


Running it:

$ ./tcsh -f
> unsetenv LANG
> set printexitvalue
> /bin/false
Exit 1, pp->p_flags & PBACKQ => 0x8 & 0x10000 => 0x0
> set i=`/bin/false`
Exit 1
> echo $i
Exit 1, pp->p_flags & PBACKQ => 0x8 & 0x10000 => 0x0
> exit
Comment 7 Jeff Bastian 2009-10-28 15:58:28 EDT
This is strange: according to gdb, the PBACKQ flag is set, but the xprintf shows that it's NOT set.

> unsetenv LANG
> set printexitvalue
> set i=`/bin/false`
Detaching after fork from child process 14239.

Breakpoint 1, pjwait (pp=0x1b54ffd0) at sh.proc.c:638
638         if ((reason != 0) && (adrof(STRprintexitvalue)) &&
(gdb) p pp->p_flags
$1 = 65544
(gdb) c
Continuing.
Exit 1
> echo $i
Exit 1, pp->p_flags & PBACKQ => 0x8 & 0x10000 => 0x0
> exit


65544 = 0x10008, which means the PBACKQ (0x10000) and PAEXITED (0x8) flags are set, thus it should not call the xprintf(), but it clearly does from the value of $i.

It must be the `/bin/false` child process that's calling xprintf().  So, maybe the PBACKQ flag is not set in the child?  Or maybe printexitvalue should be disabled in back-quoted child processes?
Comment 8 Jeff Bastian 2009-10-29 17:37:15 EDT
gdb was having problems following the child process, so I added a bunch of xprintf("DEBUG...") statements and verified that the PBACKQ flag is getting lost somewhere in the fork-and-exec calls.

I'm working on a patch to make sure the PBACKQ follows the child process.
Comment 9 Jeff Bastian 2009-10-29 18:29:39 EDT
Created attachment 366728 [details]
patch to fix backquoted output

Attached is a patch to set and preserve the F_BACKQ flag (and, in turn, the PBACKQ flag) for backquoted commands.  This fixes the bug in my testing.
Comment 12 Jeff Bastian 2009-10-30 10:02:41 EDT
I sent the patch upstream at
  http://mx.gw.com/pipermail/tcsh-bugs/2009-October/000648.html
Comment 13 Issue Tracker 2009-11-02 04:42:24 EST
Event posted on 2009-11-02 10:42 CET by rdassen

The test packages that Jeff Bastian provided for customer testing (based on
the patch from comment #9) have been successfully tested by the customer:
they confirm the test packages fix the issue and report no new problems.


This event sent from IssueTracker by rdassen 
 issue 358655