Bug 847102 - source command fails with single-line if-statement
source command fails with single-line if-statement
Product: Red Hat Enterprise Linux 6
Classification: Red Hat
Component: tcsh (Show other bugs)
i686 Linux
unspecified Severity urgent
: rc
: ---
Assigned To: Roman Kollár
Iveta Wiedermann
: Regression
: 826140 828772 857019 (view as bug list)
Depends On:
Blocks: 858281
  Show dependency treegraph
Reported: 2012-08-09 13:42 EDT by Bob Arendt
Modified: 2013-02-21 05:44 EST (History)
11 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
: 858281 (view as bug list)
Last Closed: 2013-02-21 05:44:29 EST
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

Attachments (Terms of Use)
Proposed patch to fix the source command (700 bytes, patch)
2012-08-22 11:28 EDT, Roman Kollár
no flags Details | Diff
Alternate patch for dosource() (1.79 KB, patch)
2012-08-22 18:31 EDT, Bob Arendt
no flags Details | Diff

  None (edit)
Description Bob Arendt 2012-08-09 13:42:44 EDT
Description of problem:

There is a major regression tcsh logic with this tcsh release.
A single-line if statement like:
  if (1) source /tmp/somefile.csh
fails to source the file.

This is critical, since it's *very* common to have clauses like:
  if ( -e ~/.alias ) source ~/.alias
in csh scripts.  This has a pretty high breakage footprint throughout our enterprise.

Version-Release number of selected component (if applicable):

How reproducible:  Always.  In fact, here's a trivial reproducer script "testit.csh"  Actual results:
% cat testit.csh 
echo 'echo "source worked"' > /tmp/foo.csh
echo "test start"
if (1) source /tmp/foo.csh
echo "test done"
% /bin/tcsh -f testit.csh 
test start
test done

Expected results:
% cat testit.csh
echo 'echo "source worked"' > /tmp/foo.csh
echo "test start"
if (1) source /tmp/foo.csh
echo "test done"
% /bin/tcsh -f testit.csh
test start
source worked
test done

As you can see, sourcing /tmp/foo.csh should have resulted in a "source worked" output, which it doesn't in the problem case.

Additional info:
We grabbed the source rpm and tracked it down to a problem with this patch:
Patch31: tcsh-6.17.00-history-file-locking.patch

Commenting out that patch corrects the issue.  That patch is associated with bug 648592, which unfortunately is not public.

This is pretty distressing, since this is the second time in row that a Red Hat update has gone out with a non-functioning tcsh due to a Red Hat introduced bug. The effect of RHEL 6.2 Bug 658190 "fix" was corrected with this release, but tcsh still isn't usable due to the introduction of this new logic error.
Comment 4 Bob Arendt 2012-08-12 13:37:07 EDT
Here's a quick patch to the spec file that removes the problem patch.  When a fixed version is released via RHN, please bump the release number past 20 so we'll be sure to pick up the updated package.  We've got to push this internal release to stay in production.

--- rpmbuild/SPECS/tcsh.spec_orig       2012-05-10 06:19:18.000000000 -0700
+++ rpmbuild/SPECS/tcsh.spec_revert     2012-08-09 16:41:21.000000000 -0700
@@ -3,7 +3,7 @@
 Summary: An enhanced version of csh, the C shell
 Name: tcsh
 Version: 6.17
-Release: 19%{?dist}
+Release: 20%{?dist}
 License: BSD
 Group: System Environment/Shells
 Source: ftp://ftp.astron.com/pub/tcsh/%{name}-%{version}.00.tar.gz
@@ -28,7 +28,7 @@
 Patch25: tcsh-6.17.00-child-kill-hang.patch
 Patch26: tcsh-6.17.00-variable-names.patch
 Patch30: tcsh-6.17.00-handle-signals-before-flush.patch
-Patch31: tcsh-6.17.00-history-file-locking.patch
+#Patch31: tcsh-6.17.00-history-file-locking.patch
 Patch32: tcsh-6.17.00-posix-exit-status-value.patch
 Provides: csh = %{version}
@@ -69,7 +69,7 @@
 %patch25 -p1 -b .child-kill-hang
 %patch26 -p1 -b .variable-names
 %patch30 -p1 -b .handle-signals-before-flush
-%patch31 -p1 -b .history-file-locking
+#%patch31 -p1 -b .history-file-locking
 %patch32 -p1 -b .posix-exit-status-value
 for i in Fixes WishList; do
@@ -144,6 +144,10 @@
+* Thu Aug 09 2012 Bob Arendt <rda@rincon.com> - 6.17-20
+- Remove history-file-locking.patch
+  Breaks single line "if (..) source <somefile>"  tcsh logic (#847102)
 * Thu May 10 2012 Vojtech Vitek (V-Teq) <vvitek@redhat.com> - 6.17-19
 - Revert srcfile behaviour to return backward-compatible error codes (#658074)
Comment 5 Roman Kollár 2012-08-13 09:22:11 EDT
*** Bug 826140 has been marked as a duplicate of this bug. ***
Comment 8 Roman Kollár 2012-08-22 11:28:15 EDT
Created attachment 606312 [details]
Proposed patch to fix the source command

The problem is caused by calling dosource() through a function pointer which doesn't have the flg argument. Other builtins don't need it, so I added an exception for dosource so it's now called directly.
Comment 9 Bob Arendt 2012-08-22 18:31:12 EDT
Created attachment 606398 [details]
Alternate patch for dosource()

(In reply to comment #8)
> Created attachment 606312 [details]
> Proposed patch to fix the source command

Good find - that K&R style code looks like it has declaration prototypes disabled.  It makes it easy to supply the wrong argument count.

The only issue with this and the previous history patch is that they break builds on some other platforms which *do* use prototypes:
From sh.h:
    942 #if defined(hpux) && defined(__STDC__) && !defined(__GNUC__)
    943     /* Avoid hpux ansi mode spurious warnings */
    944 typedef void (*bfunc_t) ();
    945 #else
    946 typedef void (*bfunc_t) (Char **, struct command *);
    947 #endif /* hpux && __STDC__ && !__GNUC__ */

Some non-__GNUC__ plaforms will get prototypes.  So all the commands should only have 2 args.  Although these platforms may be unsupported by RedHat, it's unlikely that upstream would accept patches that broke other architectures. Also it might be nice if prototypes were enabled for gcc;  It would certainly make maintanence easier.

Looking through the source, it looks like the thing to do is rename dosource to dosource_lck(), using the 3-arg dosource_lck() for all the direct calls in the code.  Then make a dosource() wrapper for dosource_lck() with flg=0.  So all source file reads are not locked - historically true, and should be pretty safe.

I've attached a patch that implements this.  I've built a rpm and hope to get some testing in on it.  Unfortunately we don't have a rigorous test suite .. we just seem to get bitten by bugs.
Comment 10 Roman Kollár 2012-08-28 07:55:06 EDT
*** Bug 828772 has been marked as a duplicate of this bug. ***
Comment 11 Roman Kollár 2012-09-13 09:11:10 EDT
*** Bug 857019 has been marked as a duplicate of this bug. ***
Comment 16 errata-xmlrpc 2013-02-21 05:44:29 EST
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.


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