bash 2.05 mishandles redirection with large file descriptors

[Paul Eggert]

Configuration Information [Automatically generated, do not change]:
Machine: sparc
OS: solaris2.7
Compiler: gcc
Compilation CFLAGS:  -DPROGRAM='bash' -DCONF_HOSTTYPE='sparc' 
-DCONF_OSTYPE='solaris2.7' -DCONF_MACHTYPE='sparc-sun-solaris2.7' 
-DCONF_VENDOR='sun' -DSHELL  -DHAVE_CONFIG_H   -I.  -I. -I./include -I./lib 
-I/opt/reb/include -g -O2
uname output: SunOS sic.twinsun.com 5.7 Generic_106541-15 sun4u sparc 
SUNW,UltraSPARC-IIi-Engine
Machine Type: sparc-sun-solaris2.7

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

Description:

        Bash computes file descriptors modulo the word size without
        overflow checking; this sometimes causes unexpected and
        undesirable results.

Repeat-By:

        Here's one example of the problem; I can easily come up with
        others.  This example illustrates that, when at most 64 file
        descriptors can be opened, 'bash' handles '42>' correctly
        (because it opens file descriptor 42), and it handles '429>', and
        '4294>' correctly (because those file descriptors cannot be opened).
        But it mishandles '4294967297>' by treating it as if it were '>'.
        This causes output to be unexpectedly sent to the file 'x'
        rather than to standard output.

        $ ulimit -n
        64
        $ echo '42' 42>x
        42
        $ echo '429' 429>x
        bash: x: Bad file number
        $ echo '4294' 4294>x
        bash: x: Bad file number
        $ echo '4294967297' 4294967297>x
        $ cat x
        4294967297
        $ 


Fix:

Here's a patch for the problem.  While fixing this, I fixed two
closely related bugs.  First, IS_DESCRIPTOR is no longer used (even in
bash 2.05) and references to it should be removed in the comments.
Second, there is a weird typo "if (legal_number, filename+8, &lfd)" in
redir.c.


2001-04-10  Paul Eggert  <eggert@twinsun.com>

        * command.h (REDIRECTEE.dest): Now int, not long, since it's
        used only to store file descriptors.  A negative DEST now
        denotes an out-of-range file descriptor.  All uses changed.

        * general.c (legal_number): Check for string-to-integer overflow.
        * parse.y (read_token_word): Likewise.
        * redir.c (redir_special_open, do_redirection_internal): Likewise.

        * redir.c (redirection_error): Report out-of-range file descriptors.
        Do not allocate more storage than we have to.
        (redir_special_open): Fix typo in the RF_DEVD case (wrong syntax
        for invoking legal_number).

        * print_cmd.c (cprintf): Represent an out-of-range file
        descriptor with a value that is out of range.

        * shell.h (IS_DESCRIPTOR): Remove; obsolete.

===================================================================
RCS file: RCS/command.h,v
retrieving revision 2.5
retrieving revision 2.5.0.1
diff -pu -r2.5 -r2.5.0.1
--- command.h   2000/10/05 19:31:17     2.5
+++ command.h   2001/04/11 05:56:29     2.5.0.1
@@ -95,11 +95,12 @@ typedef struct word_list {
 /*                                                                 */
 /* **************************************************************** */
 
-/* What a redirection descriptor looks like.  If FLAGS is IS_DESCRIPTOR,
-   then we use REDIRECTEE.DEST, else we use the file specified. */
-
+/* What a redirection descriptor looks like.
+   Use DEST if the instruction is r_duplicating_input or
+   r_duplicating_output, otherwise use FILENAME.
+   A negative DEST denotes an out-of-range file descriptor.  */
 typedef union {
-  long dest;                   /* Place to redirect REDIRECTOR to, or ... */
+  int dest;                    /* Place to redirect REDIRECTOR to, or ... */
   WORD_DESC *filename;         /* filename to redirect to. */
 } REDIRECTEE;
 
===================================================================
RCS file: RCS/general.c,v
retrieving revision 2.5
retrieving revision 2.5.0.1
diff -pu -r2.5 -r2.5.0.1
--- general.c   2001/02/28 18:23:24     2.5
+++ general.c   2001/04/11 05:56:29     2.5.0.1
@@ -169,7 +169,10 @@ legal_number (string, result)
   if (result)
     *result = 0;
 
+  errno = 0;
   value = strtol (string, &ep, 10);
+  if (errno != 0)
+    return (0);
 
   /* Skip any trailing whitespace, since strtol does not. */
   while (whitespace (*ep))
===================================================================
RCS file: RCS/parse.y,v
retrieving revision 2.5
retrieving revision 2.5.0.1
diff -pu -r2.5 -r2.5.0.1
--- parse.y     2001/03/27 15:06:12     2.5
+++ parse.y     2001/04/11 05:56:29     2.5.0.1
@@ -382,22 +382,22 @@ redirection:      '>' WORD
                        }
        |       GREATER_AND '-'
                        {
-                         redir.dest = 0L;
+                         redir.dest = 0;
                          $$ = make_redirection (1, r_close_this, redir);
                        }
        |       NUMBER GREATER_AND '-'
                        {
-                         redir.dest = 0L;
+                         redir.dest = 0;
                          $$ = make_redirection ($1, r_close_this, redir);
                        }
        |       LESS_AND '-'
                        {
-                         redir.dest = 0L;
+                         redir.dest = 0;
                          $$ = make_redirection (0, r_close_this, redir);
                        }
        |       NUMBER LESS_AND '-'
                        {
-                         redir.dest = 0L;
+                         redir.dest = 0;
                          $$ = make_redirection ($1, r_close_this, redir);
                        }
        |       AND_GREATER WORD
@@ -3161,7 +3161,11 @@ got_token:
                    last_read_token == LESS_AND ||
                    last_read_token == GREATER_AND))
       {
-       yylval.number = atoi (token);
+       long value;
+       if (legal_number (token, &value) && (int) value == value)
+         yylval.number = value;
+       else
+         yylval.number = -1;
        return (NUMBER);
       }
 
===================================================================
RCS file: RCS/print_cmd.c,v
retrieving revision 2.5
retrieving revision 2.5.0.1
diff -pu -r2.5 -r2.5.0.1
--- print_cmd.c 2001/04/03 19:05:26     2.5
+++ print_cmd.c 2001/04/11 05:56:29     2.5.0.1
@@ -1010,7 +1010,18 @@ cprintf (format, arg1, arg2)
              break;
 
            case 'd':
-             argp = inttostr (pointer_to_int (args[arg_index]), intbuf, sizeof 
(intbuf));
+             {
+               int i = pointer_to_int (args[arg_index]);
+               if (i < 0)
+                 {
+                   /* Represent an out-of-range file descriptor with
+                      a value that is out of range.  */
+                   sprintf (intbuf, "%u", (unsigned) -1);
+                   argp = intbuf;
+                 }
+               else
+                 argp = inttostr (i, intbuf, sizeof (intbuf));
+             }
              arg_index++;
              arg_len = strlen (argp);
              break;
===================================================================
RCS file: RCS/redir.c,v
retrieving revision 2.5
retrieving revision 2.5.0.1
diff -pu -r2.5 -r2.5.0.1
--- redir.c     2001/03/27 14:43:50     2.5
+++ redir.c     2001/04/11 05:56:29     2.5.0.1
@@ -80,6 +80,7 @@ redirection_error (temp, error)
      int error;
 {
   char *filename;
+  char *allocname = 0;
   int oflags;
 
   if (expandable_redirection_filename (temp))
@@ -89,17 +90,14 @@ redirection_error (temp, error)
          oflags = temp->redirectee.filename->flags;
          temp->redirectee.filename->flags |= W_NOGLOB;
        }
-      filename = redirection_expand (temp->redirectee.filename);
+      filename = allocname = redirection_expand (temp->redirectee.filename);
       if (posixly_correct && interactive_shell == 0)
        temp->redirectee.filename->flags = oflags;
       if (filename == 0)
-       filename = savestring (temp->redirectee.filename->word);
-      if (filename == 0)
-       {
-         filename = xmalloc (1);
-         filename[0] = '\0';
-       }
+       filename = temp->redirectee.filename->word;
     }
+  else if (temp->redirectee.dest < 0)
+    filename = "out-of-range file descriptor";
   else
     filename = itos (temp->redirectee.dest);
 
@@ -128,7 +126,7 @@ redirection_error (temp, error)
       break;
     }
 
-  FREE (filename);
+  FREE (allocname);
 }
 
 /* Perform the redirections on LIST.  If FOR_REAL, then actually make
@@ -406,8 +404,12 @@ redir_special_open (spec, filename, flag
     {
 #if !defined (HAVE_DEV_FD)
     case RF_DEVFD:
-      if (legal_number, filename+8, &lfd)
-       fd = fcntl ((int)lfd, F_DUPFD, 10);
+      if (all_digits (filename + 8))
+       {
+         if (legal_number (filename + 8, &lfd) && lfd == (int) lfd)
+           fd = lfd;
+         fd = fcntl (fd, F_DUPFD, 10);
+       }
       else
        fd = AMBIGUOUS_REDIRECT;
       break;
@@ -560,21 +562,21 @@ do_redirection_internal (redirect, for_r
        return (AMBIGUOUS_REDIRECT);
       else if (redirectee_word[0] == '-' && redirectee_word[1] == '\0')
        {
-         rd.dest = 0L;
+         rd.dest = 0;
          new_redirect = make_redirection (redirector, r_close_this, rd);
        }
       else if (all_digits (redirectee_word))
        {
-         if (ri == r_duplicating_input_word)
-           {
-             rd.dest = atol (redirectee_word);
-             new_redirect = make_redirection (redirector, r_duplicating_input, 
rd);
-           }
+         long value;
+         if (legal_number (redirectee_word, &value) && value == (int) value)
+           rd.dest = value;
          else
-           {
-             rd.dest = atol (redirectee_word);
-             new_redirect = make_redirection (redirector, 
r_duplicating_output, rd);
-           }
+           rd.dest = -1;
+         new_redirect = make_redirection (redirector,
+                                          (ri == r_duplicating_input_word
+                                           ? r_duplicating_input
+                                           : r_duplicating_output),
+                                          rd);
        }
       else if (ri == r_duplicating_output_word && redirector == 1)
        {
@@ -851,11 +853,11 @@ add_undo_redirect (fd)
 
   clexec_flag = fcntl (fd, F_GETFD, 0);
 
-  rd.dest = 0L;
+  rd.dest = 0;
   closer = make_redirection (new_fd, r_close_this, rd);
   dummy_redirect = copy_redirects (closer);
 
-  rd.dest = (long)new_fd;
+  rd.dest = new_fd;
   if (fd == 0)
     new_redirect = make_redirection (fd, r_duplicating_input, rd);
   else
@@ -891,7 +893,7 @@ add_undo_close_redirect (fd)
 {
   REDIRECT *closer;
 
-  rd.dest = 0L;
+  rd.dest = 0;
   closer = make_redirection (fd, r_close_this, rd);
   closer->next = redirection_undo_list;
   redirection_undo_list = closer;
===================================================================
RCS file: RCS/shell.h,v
retrieving revision 2.5
retrieving revision 2.5.0.1
diff -pu -r2.5 -r2.5.0.1
--- shell.h     2000/10/14 21:33:01     2.5
+++ shell.h     2001/04/11 05:56:29     2.5.0.1
@@ -41,7 +41,6 @@ extern int EOF_Reached;
 
 #define NO_PIPE -1
 #define REDIRECT_BOTH -2
-#define IS_DESCRIPTOR -1
 
 #define NO_VARIABLE -1