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

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

[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?

> 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?

> @@ -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?


-- 
ldv

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

[Alexey Appolonov]

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.

>> 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.

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.

Re: [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

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

[Ivan A. Melnikov]

On Tue, Nov 03, 2020 at 03:11:05PM +0300, Dmitry V. Levin wrote:
> 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.
[...]

IMO the interesting part here is not in read syscall semantics,
but in the types involved.

As far as I understand, result is int here, and read returs ssize_t.
If sizeof(ssize_t) > sizeof(int) (e.g. on x86_64),
given large enough input file and enough memory, read can
read more than 2^31 bytes and overflow result variable; this may lead
to such result value that (result <= file_size) will evaluate
to false (file_size is size_t, so result will be promoted to
usigned long before comparison).

It seems to me that this is possible only when result is negative,
so the first check (result >= 0) makes the second one unnecessary,
but I would not dare to call this obvious.

-- 
  wbr,
    iv m.

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

[Dmitry V. Levin]

On Tue, Nov 03, 2020 at 05:47:00PM +0400, Ivan A. Melnikov wrote:
> On Tue, Nov 03, 2020 at 03:11:05PM +0300, Dmitry V. Levin wrote:
> > 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.
> [...]
> 
> IMO the interesting part here is not in read syscall semantics,
> but in the types involved.
> 
> As far as I understand, result is int here, and read returs ssize_t.
> If sizeof(ssize_t) > sizeof(int) (e.g. on x86_64),
> given large enough input file and enough memory, read can
> read more than 2^31 bytes and overflow result variable; this may lead
> to such result value that (result <= file_size) will evaluate
> to false (file_size is size_t, so result will be promoted to
> usigned long before comparison).
> 
> It seems to me that this is possible only when result is negative,
> so the first check (result >= 0) makes the second one unnecessary,
> but I would not dare to call this obvious.

Yes, this is obvious: since the type of file_size is size_t,
the condition "(int) file_size > file_size" can be true iff
"(int) file_size < 0 && file_size < ~ (size_t) -1U"

FWIW, the upstream fix in bash-4.0-rc1 was to introduce "ssize_t nr"
and use it instead of "int result" with proper checking, see
https://git.savannah.gnu.org/cgit/bash.git/diff/builtins/evalfile.c?id=3185942a5234e26ab13fa02f9c51d340cec514f8


-- 
ldv

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

[Alexey Appolonov]

03.11.2020 15:11, Dmitry V. Levin пишет:
>>>> 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?

It is possible to reach that code by calling the "make_redirection" function
with a "dest_and_filename" parameter assigned to a REDIRECTEE var that has a
"filename" field (of type WORD_DESC) that has a "word" field that is a pointer
to a C-string "\0". I didn't investigate further than that.

>> 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.

OK, will split them.


03.11.2020 16:47, Ivan A. Melnikov пишет:
> On Tue, Nov 03, 2020 at 03:11:05PM +0300, Dmitry V. Levin wrote:
>> If read(2) could write more bytes than requested, then the buffer
>> would be overwritten before the check you're suggesting to add.
> [...]
>
> IMO the interesting part here is not in read syscall semantics,
> but in the types involved.
>
> As far as I understand, result is int here, and read returs ssize_t.
> If sizeof(ssize_t) > sizeof(int) (e.g. on x86_64),
> given large enough input file and enough memory, read can
> read more than 2^31 bytes and overflow result variable; this may lead
> to such result value that (result <= file_size) will evaluate
> to false (file_size is size_t, so result will be promoted to
> usigned long before comparison).
>
> It seems to me that this is possible only when result is negative,
> so the first check (result >= 0) makes the second one unnecessary,
> but I would not dare to call this obvious.

Well, yeah, that's the type of thing I was thinking about.