From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13527 invoked by alias); 28 Mar 2018 11:20:52 -0000 Mailing-List: contact zsh-workers-help@zsh.org; run by ezmlm Precedence: bulk X-No-Archive: yes List-Id: Zsh Workers List List-Post: List-Help: List-Unsubscribe: X-Seq: 42553 Received: (qmail 12841 invoked by uid 1010); 28 Mar 2018 11:20:52 -0000 X-Qmail-Scanner-Diagnostics: from mx3-rdu2.redhat.com by f.primenet.com.au (envelope-from , uid 7791) with qmail-scanner-2.11 (clamdscan: 0.99.2/21882. spamassassin: 3.4.1. Clear:RC:0(66.187.233.73):SA:0(-1.9/5.0):. Processed in 15.202624 secs); 28 Mar 2018 11:20:52 -0000 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on f.primenet.com.au X-Spam-Level: X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,T_RP_MATCHES_RCVD, T_SPF_HELO_PERMERROR,T_SPF_TEMPERROR autolearn=ham autolearn_force=no version=3.4.1 X-Envelope-From: kdudka@redhat.com X-Qmail-Scanner-Mime-Attachments: | X-Qmail-Scanner-Zip-Files: | From: Kamil Dudka To: Oliver Kiddle Cc: zsh-workers@zsh.org Subject: Re: PATCH: spelling correction buffer overflow Date: Wed, 28 Mar 2018 13:20:56 +0200 Message-ID: <2328442.7oMNVitVLA@kdudka-nb> In-Reply-To: <52497193.Jvz9bWjiNL@kdudka-nb> References: <3579.1522103995@thecus> <52497193.Jvz9bWjiNL@kdudka-nb> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Wed, 28 Mar 2018 11:20:33 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Wed, 28 Mar 2018 11:20:33 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'kdudka@redhat.com' RCPT:'' On Tuesday, March 27, 2018 11:55:50 PM CEST Kamil Dudka wrote: > On Tuesday, March 27, 2018 12:39:55 AM CEST Oliver Kiddle wrote: > > In utils.c:spname(), there's no checking on newname[] for overflows in > > two out the of three places where it is appended to. > > Thanks! After applying this patch, zsh does not crash any more while > trying to correct an overly long command. I spotted that this patch introduced new compiler warnings: Src/utils.c:4430:26: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] # return (new - newname) >= (sizeof(newname)-1) ? NULL : newname; # ^ Src/utils.c:4438:26: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] # if ((new - newname) >= (sizeof(newname)-1)) # ^ The full report is available at: https://travis-ci.org/kdudka/zsh/builds/359289437#L2837 Should we cast RHS of the >= operator to ssize_t or ptrdiff_t to avoid them? > Kamil > > > returning NULL appears to abort the command-line rather than aborting > > only correction which is perhaps not ideal but the aim here is not to > > corrupt other bits of memory. > > > > This also tweaks the struncpy() function I updated in the earlier patch. > > > > Oliver > > > > diff --git a/Src/utils.c b/Src/utils.c > > index 998b16220..9ea34ab54 100644 > > --- a/Src/utils.c > > +++ b/Src/utils.c > > @@ -2283,7 +2283,8 @@ struncpy(char **s, char *t, int n) > > > > { > > > > char *u = *s; > > > > - while (n-- && (*u++ = *t++)); > > + while (n-- && (*u = *t++)) > > + u++; > > > > *s = u; > > if (n > 0) /* just one null-byte will do, unlike strncpy(3) */ > > > > *u = '\0'; > > > > @@ -4420,17 +4421,20 @@ spname(char *oldname) > > > > * odd to the human reader, and we may make use of the total * > > * distance for all corrections at some point in the future. */ > > > > if (bestdist < maxthresh) { > > > > - strcpy(new, spnameguess); > > - strcat(new, old); > > - return newname; > > + struncpy(&new, spnameguess, sizeof(newname) - (new - newname)); > > + struncpy(&new, old, sizeof(newname) - (new - newname)); > > + return (new - newname) >= (sizeof(newname)-1) ? NULL : newname; > > > > } else > > > > return NULL; > > > > } else { > > > > maxthresh = bestdist + thresh; > > bestdist += thisdist; > > > > } > > > > - for (p = spnamebest; (*new = *p++);) > > + for (p = spnamebest; (*new = *p++);) { > > + if ((new - newname) >= (sizeof(newname)-1)) > > + return NULL; > > > > new++; > > > > + } > > > > } > > > > }