From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 4 Nov 2020 01:01:09 +0300 From: "Dmitry V. Levin" To: ALT Linux Team development discussions Message-ID: <20201103220108.GA8778@altlinux.org> References: <20201030213543.9C351844021E@gitery.altlinux.org> <20201102221840.GA24969@altlinux.org> <20201103121105.GA1841@altlinux.org> <20201103134700.viigwhhxgqbswdki@titan.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=koi8-r Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20201103134700.viigwhhxgqbswdki@titan.localdomain> Cc: "Ivan A. Melnikov" , Alexey Appolonov Subject: Re: [devel] [SCM] packages/bash3: heads/p9 X-BeenThere: devel@lists.altlinux.org X-Mailman-Version: 2.1.12 Precedence: list Reply-To: ALT Linux Team development discussions List-Id: ALT Linux Team development discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 03 Nov 2020 22:01:09 -0000 Archived-At: List-Archive: List-Post: 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 > > > >> 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 > > > >> 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