[devel] [SCM] packages/bash3: heads/p9

[Dmitry V. Levin]

On Tue, Nov 03, 2020 at 04:32:08AM +0300, Alexey Appolonov wrote:
> 03.11.2020 01:18, Dmitry V. Levin пишет:
> > On Fri, Oct 30, 2020 at 09:35:43PM +0000, Alexey Appolonov wrote:
> >> Update of /people/alexey/packages/bash3.git
> > [...]
> >> commit 0db3057724a11b6e9f3bdca6a831370443aaa06e
> >> Author: Alexey Appolonov <alexey на altlinux.org>
> >> Date:   Tue Oct 27 14:36:56 2020 +0300
> >>
> >>      Preventing a segmentation fault in '_evalfile' func of 'evalfile.c'
> >>      
> >>      Index 'result' can be -1; The index is checked from the top also
> >>      (just in case).
> > Prevent a potential segmentation fault ...
> >
> >> commit 6c6399d5a7cb51ae53c196f0bfdfd92e43711544
> >> Author: Alexey Appolonov <alexey на altlinux.org>
> >> Date:   Tue Oct 27 13:53:08 2020 +0300
> >>
> >>      Preventing a segmentation fault in 'make_redirection' func of 'make_cmd.c'
> >>      
> >>      Index 'wlen' is -1 if a length of 'w->word' C-string is 0.
> > Likewise.
> >
> > [...]
> >> diff --git a/bash/builtins/evalfile.c b/bash/builtins/evalfile.c
> >> index d05bc7b..9eec1a5 100644
> >> --- a/bash/builtins/evalfile.c
> >> +++ b/bash/builtins/evalfile.c
> >> @@ -149,7 +149,8 @@ file_error_and_exit:
> >>   
> >>     string = (char *)xmalloc (1 + file_size);
> >>     result = read (fd, string, file_size);
> >> -  string[result] = '\0';
> >> +  if (result >= 0 && result <= file_size)
> >> +    string[result] = '\0';
> >>   
> >>     return_val = errno;
> >>     close (fd);
> > Adding a check for result <= file_size?  Really?  Do you suppose that
> > read (fd, string, file_size) is ever capable of returning a value
> > greater than file_size?
> 
> I just think that the comfort of having such checks outweighs
> the negligible computational costs that they bring.

If read(2) could write more bytes than requested, then the buffer
would be overwritten before the check you're suggesting to add.

The cost of proposing such checks is way too high: they make people doubt
whether other proposed changes worth reviewing.

> >> diff --git a/bash/make_cmd.c b/bash/make_cmd.c
> >> index c819b27..bb33da2 100644
> >> --- a/bash/make_cmd.c
> >> +++ b/bash/make_cmd.c
> >> @@ -731,7 +731,12 @@ make_redirection (source, instruction, dest_and_filename)
> >>       case r_duplicating_output_word:	/* 1>&$foo */
> >>         w = dest_and_filename.filename;
> >>         wlen = strlen (w->word) - 1;
> >> -      if (w->word[wlen] == '-')		/* Yuck */
> >> +      if (wlen < 0)
> >> +        {
> >> +          programming_error (_("make_redirection: redirection instruction `%d' for an empty file name"), instruction);
> >> +          abort ();
> >> +        }
> >> +      else if (w->word[wlen] == '-')	/* Yuck */
> >>           {
> >>             w->word[wlen] = '\0';
> >>   	  if (all_digits (w->word) && legal_number (w->word, &lfd) && lfd == (int)lfd)
> > A programming error?  Is it ever possible to reach the code added by this hunk?
> 
> It is if "strlen (w->word)" equals 0.

Is it theoretically possible that "strlen (w->word) equals 0"?  In other
words, is the proposed code reachable, or are you adding this just to
pacify the unnamed static code analysis tool?

> The "programming_error" function is used to handle another unlikely scenario
> (see just below in the code).
> 
> >> @@ -743,7 +748,6 @@ make_redirection (source, instruction, dest_and_filename)
> >>   	  else
> >>   	    temp->instruction = (instruction == r_duplicating_input_word) ? r_move_input_word : r_move_output_word;
> >>           }
> >> -
> >>         break;
> >>   
> >>       default:
> > Do you really need to change spacings in bash3?
> 
> I find it quite distracting in this particular case. And it didn't match
> the code style anyway.

Please avoid mixing unrelated changes.


-- 
ldv