From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 4077 invoked by alias); 29 Mar 2018 08:42:32 -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: 42562 Received: (qmail 13231 invoked by uid 1010); 29 Mar 2018 08:42:32 -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(-4.2/5.0):. Processed in 2.915092 secs); 29 Mar 2018 08:42:32 -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=-4.2 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_MED, SPF_PASS,T_RP_MATCHES_RCVD,T_SPF_HELO_PERMERROR 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: Thu, 29 Mar 2018 10:42:47 +0200 Message-ID: <3166008.q5aWb97ZyJ@kdudka-nb> In-Reply-To: <11608.1522246044@thecus> References: <3579.1522103995@thecus> <2328442.7oMNVitVLA@kdudka-nb> <11608.1522246044@thecus> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="nextPart7397551.tsDiMBGsFb" Content-Transfer-Encoding: 7Bit X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Thu, 29 Mar 2018 08:42:24 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Thu, 29 Mar 2018 08:42:24 +0000 (UTC) for IP:'10.11.54.5' DOMAIN:'int-mx05.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'kdudka@redhat.com' RCPT:'' --nextPart7397551.tsDiMBGsFb Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" On Wednesday, March 28, 2018 4:07:24 PM CEST Oliver Kiddle wrote: > Kamil Dudka wrote: > -Wsign-compare is not on by default and turning it on results in quite a > few warnings for the zsh code. I know but it is not a reason to introduce new warnings. > It seems a little silly given that the > compiler ought to be able evaluate sizeof() at compile time and > establish that it is within the range of a signed integer. The compiler knows it. However, the range of a signed integer is not the problem that the compiler warns you about. The problem is that semantics of the C language is not exactly intuitive here. A real-world problem would occur if (new - newname) became negative (not sure if the surrounding code allows it or not). Then the condition would be evaluated as if the negative number was higher than the positive number on RHS. Please have a look at the attached example. It demonstrates the actual problem that the compiler warns you about. Interestingly, this is not an undefined behavior. It is mandated (for example) by the C99 specification. > > Should we cast RHS of the >= operator to ssize_t or ptrdiff_t to avoid > > them? > Casts are a bit ugly. The following - adding newname to both the LHS > and RHS - appears to avoid the warning by making both sides of the > comparison of type signed. Looks good to me. Kamil > At least until someone decides that adding an > unsigned to the signed should also generate a warning. > > Oliver > > diff --git a/Src/utils.c b/Src/utils.c > index eab407eee..3587c3622 100644 > --- a/Src/utils.c > +++ b/Src/utils.c > @@ -4396,7 +4396,7 @@ spname(char *oldname) > * Rationale for this, if there ever was any, has been forgotten. */ > for (;;) { > while (*old == '/') { > - if ((new - newname) >= (sizeof(newname)-1)) > + if (new >= newname + sizeof(newname) - 1) > return NULL; > *new++ = *old++; > } > @@ -4427,7 +4427,7 @@ spname(char *oldname) > if (bestdist < maxthresh) { > struncpy(&new, spnameguess, sizeof(newname) - (new - newname)); > struncpy(&new, old, sizeof(newname) - (new - newname)); > - return (new - newname) >= (sizeof(newname)-1) ? NULL : newname; > + return (new >= newname + sizeof(newname) - 1) ? NULL : newname; > } else > return NULL; > } else { > @@ -4435,7 +4435,7 @@ spname(char *oldname) > bestdist += thisdist; > } > for (p = spnamebest; (*new = *p++);) { > - if ((new - newname) >= (sizeof(newname)-1)) > + if (new >= newname + sizeof(newname) - 1) > return NULL; > new++; > } --nextPart7397551.tsDiMBGsFb Content-Disposition: attachment; filename="sign-compare.c" Content-Transfer-Encoding: 7Bit Content-Type: text/x-csrc; charset="UTF-8"; name="sign-compare.c" #include #include int main() { const char *p1 = "text"; const char *p2 = p1 + 2; ptrdiff_t diff = p1 - p2; printf("diff = %td\n", diff); printf("sizeof(p2) - 1 = %zu\n", sizeof(p2) - 1); if (diff >= sizeof(p2) - 1) printf("diff >= sizeof(p2) - 1\n"); else printf("diff < sizeof(p2) - 1\n"); } --nextPart7397551.tsDiMBGsFb--