From: "Dmitry V. Levin" <ldv@altlinux.org> To: Alexey Appolonov <alexey@altlinux.org> Cc: devel@lists.altlinux.org Subject: Re: [devel] [SCM] packages/bash3: heads/p9 Date: Tue, 3 Nov 2020 15:11:05 +0300 Message-ID: <20201103121105.GA1841@altlinux.org> (raw) In-Reply-To: <d85bd5b9-86a2-eac2-88fe-fa357367e8e6@altlinux.org> 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
next prev parent reply other threads:[~2020-11-03 12:11 UTC|newest] Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-11-02 22:18 ` Dmitry V. Levin 2020-11-03 1:32 ` Alexey Appolonov 2020-11-03 12:11 ` Dmitry V. Levin [this message] 2020-11-03 13:47 ` Ivan A. Melnikov 2020-11-03 22:01 ` Dmitry V. Levin 2020-11-03 14:34 ` Alexey Appolonov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20201103121105.GA1841@altlinux.org \ --to=ldv@altlinux.org \ --cc=alexey@altlinux.org \ --cc=devel@lists.altlinux.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
ALT Linux Team development discussions This inbox may be cloned and mirrored by anyone: git clone --mirror http://lore.altlinux.org/devel/0 devel/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 devel devel/ http://lore.altlinux.org/devel \ devel@altlinux.org devel@altlinux.ru devel@lists.altlinux.org devel@lists.altlinux.ru devel@linux.iplabs.ru mandrake-russian@linuxteam.iplabs.ru sisyphus@linuxteam.iplabs.ru public-inbox-index devel Example config snippet for mirrors. Newsgroup available over NNTP: nntp://lore.altlinux.org/org.altlinux.lists.devel AGPL code for this site: git clone https://public-inbox.org/public-inbox.git