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

[Alexey Appolonov <alexey@altlinux.org>]

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.