PATCH: make_var_array() mishandles exportstr on CYGWIN and SuSE

[Wolfgang Oertl]

Configuration Information [Automatically generated, do not change]:
Machine: i386
OS: linux
Compiler: gcc
Compilation CFLAGS:  -DPROGRAM='bash' -DCONF_HOSTTYPE='i386' 
-DCONF_OSTYPE='linux' -DCONF_MACHTYPE='i386-suse-linux' -DCONF_VENDOR='suse' 
-DSHELL -DHAVE_CONFIG_H  -D_FILE_OFFSET_BITS=64  -I. -I/usr/include -I. 
-I./include -I./lib -I/usr/include -O2 -m486 -D_GNU_SOURCE -Wall -pipe
uname output: Linux laptop 2.4.0-test10 #24 Tue Nov 7 22:56:09 CET 2000 i686 
unknown
Machine Type: i386-suse-linux

Bash Version: 2.04
Patch Level: 0
Release Status: release

Description:
        In the vanilla version this problem only arises on __CYGWIN__ and
        __CYGWIN32__ platforms.  The Bash distributed with SuSE 7.0 contains a
        patch that triggers this problem by enabling the __CYGWIN__ code in
        this function.

        During generation of 'export_env', function variables that have their
        'exportstr' set, lose.  The problem is localized to the function
        variables.c:make_var_array().

        Consider this code fragment of variables.c:make_var_array():

>       if (var->exportstr)
>         {
> #if defined(__CYGWIN__) || defined (__CYGWIN32__)
>         INVALIDATE_EXPORTSTR (var);
>         value = value_cell (var);
> #else
>         /* XXX -- this test can go away in the next release, to be replaced
>            by a simple `value = var->exportstr;', when the exportstr code
>            is better-tested.  Until then, don't do it for cygwin at all,
>            since that system has some weird environment variables. */
>         if (valid_exportstr (var))
>           value = var->exportstr;
>         else
>           {
>             INVALIDATE_EXPORTSTR (var);
>             value = value_cell (var);
>           }
> #endif
>         }

        There are some ways for 'value = value_cell (var)' to happen, which is
        bad when 'var' is a function.  Actually, 'exportstr' is set only for
        functions!

Repeat-By:
        $ function foo_func () { echo here; }
        $ export -f foo_func
        $ /bin/ls
        $ export BAR=1
        $ sh
        $ set

        /bin/ls (or whatever external command) provokes a call of
        'maybe_make_export_env', which makes 'export_env', thereby setting the
        'exportstr' of 'foo_func'.

        The second time (provoked by changing an exported variable, BAR in
        this case, and then another external command), the function
        'make_var_array' behaves badly and uses 'value_cell (var)' on the
        function variable!  Only 'function_cell (var)' must be used...  This
        results in a Bad String.

        In set's output in the subshell, the 'foo_func' is listed as a string
        variable with a content of "\t" (which is, in my setup, the result of
        using a pointer to COMMAND (4 bytes) as a string).

Fix:
        A quick fix would be to replace the instances of 'value_cell (var)' in
        the code fragment above with 'named_function_string ((char *)NULL,
        function_cell (var), 0)'.

        This would break as soon as arrays can be exported, though.  This
        patch (for the unpatched version of Bash) should yield better
        results in the long run.  It still leaves the call to
        'valid_exportstr' intact, which (IMHO) could be removed.
 
--- variables.c.orig    Thu Nov 16 00:01:30 2000
+++ variables.c Thu Nov 16 16:55:45 2000
@@ -2492,30 +2492,33 @@
 
   list = alloc_array ((1 + array_len ((char **)vars)));
 
-#define USE_EXPORTSTR (value == var->exportstr)
-
   for (i = 0, list_index = 0; (var = vars[i]); i++)
     {
+      /* When var->exportstr is set, assume that it is up to date.  All
+       * changes to variables must therefore call INVALIDATE_EXPORTSTR.
+       */
       if (var->exportstr)
-        {
+       {
 #if defined(__CYGWIN__) || defined (__CYGWIN32__)
          INVALIDATE_EXPORTSTR (var);
-         value = value_cell (var);
 #else
-         /* XXX -- this test can go away in the next release, to be replaced
-            by a simple `value = var->exportstr;', when the exportstr code
+         /* XXX -- this test can go away in the next release
+            when the exportstr code
             is better-tested.  Until then, don't do it for cygwin at all,
             since that system has some weird environment variables. */
           if (valid_exportstr (var))
-           value = var->exportstr;
-         else
            {
-             INVALIDATE_EXPORTSTR (var);
-             value = value_cell (var);
+             /* Gee, I'd like to get away with not using savestring() if we're
+                using the cached exportstr... */
+             list[list_index++] = savestring (var->exportstr);
+             continue;
            }
+         INVALIDATE_EXPORTSTR (var);
 #endif
-        }
-      else if (function_p (var))
+       }
+
+      /* otherwise, get the current value */
+      if (function_p (var))
        value = named_function_string ((char *)NULL, function_cell (var), 0);
 #if defined (ARRAY_VARS)
       else if (array_p (var))
@@ -2530,17 +2533,18 @@
 
       if (value)
        {
-         /* Gee, I'd like to get away with not using savestring() if we're
-            using the cached exportstr... */
-         list[list_index] = USE_EXPORTSTR ? savestring (value)
-                                          : mk_env_string (var->name, value);
+         list[list_index] = mk_env_string (var->name, value);
 
-         if (USE_EXPORTSTR == 0 && function_p (var))
+         /* This place is reached only if var->exportstr is NULL */
+         if (function_p (var)
+#if defined (ARRAY_VARS) && 0
+                 || array_p (var)
+#endif
+                 )
            {
              SAVE_EXPORTSTR (var, list[list_index]);
            }
          list_index++;
-#undef USE_EXPORTSTR
 
 #if 0  /* not yet */
 #if defined (ARRAY_VARS)
        


Details
=======

Of course the same happens when a shell script defines and exports functions,
and then calls other scripts (that was how I noticed it).


The first time 'maybe_make_export_env' is called (with array_needs_making
set), the shell variable for 'func' looks like this (in gdb):

$41 = {name = 0x80d53ac "func", value = 0x80d684c "\t", exportstr = 0x0, 
dynamic_value = 0,
  assign_func = 0, attributes = 33, context = 0, prev_context = 0x0}

The second time:

$43 = {name = 0x80d53ac "func", value = 0x80d684c "\t",
  exportstr = 0x80d694c "func=() {  echo \"here is the function\"\n}", 
dynamic_value = 0,
  assign_func = 0, attributes = 33, context = 0, prev_context = 0x0}


As you can see, the only difference is that 'exportstr' is already computed.

The function make_var_array() looks like this in my installed Bash.
(It includes a patch at this point, enabling the #if defined __CYGWIN__ part,
by andreas.schwab@suse.de, dated 2000/03/27.  Without the patch the problem
wouldn't arise, and INVALIDATE_EXPORTSTR wouldn't be called.)

> #define USE_EXPORTSTR (value == var->exportstr)
>  
>   for (i = 0, list_index = 0; (var = vars[i]); i++)
>     {
>       if (var->exportstr)
>         {
>           INVALIDATE_EXPORTSTR (var);
>           value = value_cell (var);
>         }
>       else if (function_p (var))
>         value = named_function_string ((char *)NULL, function_cell (var), 0);
> #if defined (ARRAY_VARS)
>       else if (array_p (var))
> #  if 0
>         value = array_to_assignment_string (array_cell (var));
> #  else
>         continue;       /* XXX array vars cannot yet be exported */
> #  endif
> #endif
>       else
>         value = value_cell (var);

exportstr is used only for functions: the only time SAVE_EXPORTSTR is called
is in this very function, enclosed by an if(function_p()) predicate.

What follows is that when var->exportstr is set (only for functions), you end
up with 'value = value_cell (var)', which is *not* what you want...  This in
fact uses the pointer to a COMMAND structure as a string.

end