Message ID | 20200616122420.5175-1-stli@linux.ibm.com |
---|---|
State | Committed |
Headers |
Return-Path: <libc-alpha-bounces@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id D4FD2388C008; Tue, 16 Jun 2020 12:24:36 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org D4FD2388C008 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1592310276; bh=qv0sgFcPNaWuy8tfaZ6yjKFO4JR5W9Q2EWADXL2NXXo=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=ow4U2XeFQO2CefzzSf2/9FL+wLFHFTYpcnOnW9woRm5iZI45PfpoqjQiZvKHhGu/y phdgEDheodBVPxyNsu2x0M1gCNbeR7BYKgnVtpLZR0rrsUxxEkHlvnJbRP3WYaSvKf lLXKbjqY54MAJNZBOIt0MUxfNGDmh9F15K/ZwJik= X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com [148.163.156.1]) by sourceware.org (Postfix) with ESMTPS id 77D993887010 for <libc-alpha@sourceware.org>; Tue, 16 Jun 2020 12:24:33 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 77D993887010 Received: from pps.filterd (m0098399.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.42/8.16.0.42) with SMTP id 05GC32YN102362 for <libc-alpha@sourceware.org>; Tue, 16 Jun 2020 08:24:32 -0400 Received: from ppma04ams.nl.ibm.com (63.31.33a9.ip4.static.sl-reverse.com [169.51.49.99]) by mx0a-001b2d01.pphosted.com with ESMTP id 31nres5wdg-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for <libc-alpha@sourceware.org>; Tue, 16 Jun 2020 08:24:32 -0400 Received: from pps.filterd (ppma04ams.nl.ibm.com [127.0.0.1]) by ppma04ams.nl.ibm.com (8.16.0.42/8.16.0.42) with SMTP id 05GCFfuo002379 for <libc-alpha@sourceware.org>; Tue, 16 Jun 2020 12:24:30 GMT Received: from b06cxnps4076.portsmouth.uk.ibm.com (d06relay13.portsmouth.uk.ibm.com [9.149.109.198]) by ppma04ams.nl.ibm.com with ESMTP id 31mpe7wacu-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for <libc-alpha@sourceware.org>; Tue, 16 Jun 2020 12:24:30 +0000 Received: from d06av21.portsmouth.uk.ibm.com (d06av21.portsmouth.uk.ibm.com [9.149.105.232]) by b06cxnps4076.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 05GCORhk22479102 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 16 Jun 2020 12:24:27 GMT Received: from d06av21.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 5C8E752065; Tue, 16 Jun 2020 12:24:27 +0000 (GMT) Received: from oc4452167425.ibm.com (unknown [9.145.183.148]) by d06av21.portsmouth.uk.ibm.com (Postfix) with ESMTP id 2D7A352059; Tue, 16 Jun 2020 12:24:27 +0000 (GMT) To: libc-alpha@sourceware.org Subject: [PATCH] Fix stringop-overflow errors from gcc 10 in iconv. Date: Tue, 16 Jun 2020 14:24:20 +0200 Message-Id: <20200616122420.5175-1-stli@linux.ibm.com> X-Mailer: git-send-email 2.25.0 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-TM-AS-GCONF: 00 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.216, 18.0.687 definitions=2020-06-16_04:2020-06-16, 2020-06-16 signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 mlxscore=0 clxscore=1015 mlxlogscore=858 suspectscore=1 phishscore=0 bulkscore=0 adultscore=0 lowpriorityscore=0 priorityscore=1501 spamscore=0 cotscore=-2147483648 malwarescore=0 impostorscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2004280000 definitions=main-2006160088 X-Spam-Status: No, score=-13.4 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list <libc-alpha.sourceware.org> List-Unsubscribe: <http://sourceware.org/mailman/options/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-request@sourceware.org?subject=help> List-Subscribe: <http://sourceware.org/mailman/listinfo/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=subscribe> From: Stefan Liebler via Libc-alpha <libc-alpha@sourceware.org> Reply-To: Stefan Liebler <stli@linux.ibm.com> Cc: Stefan Liebler <stli@linux.ibm.com> Errors-To: libc-alpha-bounces@sourceware.org Sender: "Libc-alpha" <libc-alpha-bounces@sourceware.org> |
Series |
Fix stringop-overflow errors from gcc 10 in iconv.
|
|
Commit Message
Stefan Liebler
June 16, 2020, 12:24 p.m. UTC
On s390x, I've recognize various -Werror=stringop-overflow messages in iconv/loop.c and iconv/skeleton.c if build with gcc10 -O3. With this commit gcc knows the size and do not raise those errors anymore. --- iconv/loop.c | 14 ++++++++------ iconv/skeleton.c | 8 +++++--- 2 files changed, 13 insertions(+), 9 deletions(-)
Comments
ping On 6/16/20 2:24 PM, Stefan Liebler wrote: > On s390x, I've recognize various -Werror=stringop-overflow messages > in iconv/loop.c and iconv/skeleton.c if build with gcc10 -O3. > > With this commit gcc knows the size and do not raise those errors anymore. > --- > iconv/loop.c | 14 ++++++++------ > iconv/skeleton.c | 8 +++++--- > 2 files changed, 13 insertions(+), 9 deletions(-) > > diff --git a/iconv/loop.c b/iconv/loop.c > index 9f7570d591..b032fcd9ac 100644 > --- a/iconv/loop.c > +++ b/iconv/loop.c > @@ -420,8 +420,10 @@ SINGLE(LOOPFCT) (struct __gconv_step *step, > # else > /* We don't have enough input for another complete input > character. */ > - while (inptr < inend) > - state->__value.__wchb[inlen++] = *inptr++; > + size_t inlen_after = inlen + (inend - inptr); > + assert (inlen_after <= sizeof (state->__value.__wchb)); > + for (; inlen < inlen_after; inlen++) > + state->__value.__wchb[inlen] = *inptr++; > # endif > > return __GCONV_INCOMPLETE_INPUT; > @@ -483,11 +485,11 @@ SINGLE(LOOPFCT) (struct __gconv_step *step, > /* We don't have enough input for another complete input > character. */ > assert (inend - inptr > (state->__count & ~7)); > - assert (inend - inptr <= sizeof (state->__value)); > + assert (inend - inptr <= sizeof (state->__value.__wchb)); > state->__count = (state->__count & ~7) | (inend - inptr); > - inlen = 0; > - while (inptr < inend) > - state->__value.__wchb[inlen++] = *inptr++; > + for (inlen = 0; inlen < inend - inptr; inlen++) > + state->__value.__wchb[inlen] = inptr[inlen]; > + inptr = inend; > # endif > } > > diff --git a/iconv/skeleton.c b/iconv/skeleton.c > index 1dc642e2fc..1a38b51a5a 100644 > --- a/iconv/skeleton.c > +++ b/iconv/skeleton.c > @@ -795,11 +795,13 @@ FUNCTION_NAME (struct __gconv_step *step, struct __gconv_step_data *data, > # else > /* Make sure the remaining bytes fit into the state objects > buffer. */ > - assert (inend - *inptrp < 4); > + size_t cnt_after = inend - *inptrp; > + assert (cnt_after <= sizeof (data->__statep->__value.__wchb)); > > size_t cnt; > - for (cnt = 0; *inptrp < inend; ++cnt) > - data->__statep->__value.__wchb[cnt] = *(*inptrp)++; > + for (cnt = 0; cnt < cnt_after; ++cnt) > + data->__statep->__value.__wchb[cnt] = (*inptrp)[cnt]; > + *inptrp = inend; > data->__statep->__count &= ~7; > data->__statep->__count |= cnt; > # endif >
Hi Stefan, On 6/16/20 9:24 AM, Stefan Liebler via Libc-alpha wrote: > On s390x, I've recognize various -Werror=stringop-overflow messages > in iconv/loop.c and iconv/skeleton.c if build with gcc10 -O3. > > With this commit gcc knows the size and do not raise those errors anymore. > --- > iconv/loop.c | 14 ++++++++------ > iconv/skeleton.c | 8 +++++--- > 2 files changed, 13 insertions(+), 9 deletions(-) > > diff --git a/iconv/loop.c b/iconv/loop.c > index 9f7570d591..b032fcd9ac 100644 > --- a/iconv/loop.c > +++ b/iconv/loop.c > @@ -420,8 +420,10 @@ SINGLE(LOOPFCT) (struct __gconv_step *step, > # else > /* We don't have enough input for another complete input > character. */ > - while (inptr < inend) > - state->__value.__wchb[inlen++] = *inptr++; > + size_t inlen_after = inlen + (inend - inptr); > + assert (inlen_after <= sizeof (state->__value.__wchb)); > + for (; inlen < inlen_after; inlen++) > + state->__value.__wchb[inlen] = *inptr++; > # endif > > return __GCONV_INCOMPLETE_INPUT; > @@ -483,11 +485,11 @@ SINGLE(LOOPFCT) (struct __gconv_step *step, > /* We don't have enough input for another complete input > character. */ > assert (inend - inptr > (state->__count & ~7)); > - assert (inend - inptr <= sizeof (state->__value)); > + assert (inend - inptr <= sizeof (state->__value.__wchb)); > state->__count = (state->__count & ~7) | (inend - inptr); > - inlen = 0; > - while (inptr < inend) > - state->__value.__wchb[inlen++] = *inptr++; > + for (inlen = 0; inlen < inend - inptr; inlen++) > + state->__value.__wchb[inlen] = inptr[inlen]; > + inptr = inend; > # endif > } > > diff --git a/iconv/skeleton.c b/iconv/skeleton.c > index 1dc642e2fc..1a38b51a5a 100644 > --- a/iconv/skeleton.c > +++ b/iconv/skeleton.c > @@ -795,11 +795,13 @@ FUNCTION_NAME (struct __gconv_step *step, struct __gconv_step_data *data, > # else > /* Make sure the remaining bytes fit into the state objects > buffer. */ > - assert (inend - *inptrp < 4); > + size_t cnt_after = inend - *inptrp; > + assert (cnt_after <= sizeof (data->__statep->__value.__wchb)); > > size_t cnt; > - for (cnt = 0; *inptrp < inend; ++cnt) > - data->__statep->__value.__wchb[cnt] = *(*inptrp)++; > + for (cnt = 0; cnt < cnt_after; ++cnt) > + data->__statep->__value.__wchb[cnt] = (*inptrp)[cnt]; > + *inptrp = inend; > data->__statep->__count &= ~7; > data->__statep->__count |= cnt; > # endif > Thanks for working on this! I also noticed this same issue on power. This patch indeed fixes it there as well. As for the patch, I'm not that familiar with iconv code, but by checking the modified snippet, the loops seem equivalent and the pointers are properly modified as before. So it's mostly harmless, basically rewriting those lines in a different way to please GCC. LGTM. -- Matheus Castanho
On 6/26/20 8:00 PM, Matheus Castanho wrote: > Hi Stefan, > > On 6/16/20 9:24 AM, Stefan Liebler via Libc-alpha wrote: >> On s390x, I've recognize various -Werror=stringop-overflow messages >> in iconv/loop.c and iconv/skeleton.c if build with gcc10 -O3. >> >> With this commit gcc knows the size and do not raise those errors anymore. >> --- >> iconv/loop.c | 14 ++++++++------ >> iconv/skeleton.c | 8 +++++--- >> 2 files changed, 13 insertions(+), 9 deletions(-) >> >> diff --git a/iconv/loop.c b/iconv/loop.c >> index 9f7570d591..b032fcd9ac 100644 >> --- a/iconv/loop.c >> +++ b/iconv/loop.c >> @@ -420,8 +420,10 @@ SINGLE(LOOPFCT) (struct __gconv_step *step, >> # else >> /* We don't have enough input for another complete input >> character. */ >> - while (inptr < inend) >> - state->__value.__wchb[inlen++] = *inptr++; >> + size_t inlen_after = inlen + (inend - inptr); >> + assert (inlen_after <= sizeof (state->__value.__wchb)); >> + for (; inlen < inlen_after; inlen++) >> + state->__value.__wchb[inlen] = *inptr++; >> # endif >> >> return __GCONV_INCOMPLETE_INPUT; >> @@ -483,11 +485,11 @@ SINGLE(LOOPFCT) (struct __gconv_step *step, >> /* We don't have enough input for another complete input >> character. */ >> assert (inend - inptr > (state->__count & ~7)); >> - assert (inend - inptr <= sizeof (state->__value)); >> + assert (inend - inptr <= sizeof (state->__value.__wchb)); >> state->__count = (state->__count & ~7) | (inend - inptr); >> - inlen = 0; >> - while (inptr < inend) >> - state->__value.__wchb[inlen++] = *inptr++; >> + for (inlen = 0; inlen < inend - inptr; inlen++) >> + state->__value.__wchb[inlen] = inptr[inlen]; >> + inptr = inend; >> # endif >> } >> >> diff --git a/iconv/skeleton.c b/iconv/skeleton.c >> index 1dc642e2fc..1a38b51a5a 100644 >> --- a/iconv/skeleton.c >> +++ b/iconv/skeleton.c >> @@ -795,11 +795,13 @@ FUNCTION_NAME (struct __gconv_step *step, struct __gconv_step_data *data, >> # else >> /* Make sure the remaining bytes fit into the state objects >> buffer. */ >> - assert (inend - *inptrp < 4); >> + size_t cnt_after = inend - *inptrp; >> + assert (cnt_after <= sizeof (data->__statep->__value.__wchb)); >> >> size_t cnt; >> - for (cnt = 0; *inptrp < inend; ++cnt) >> - data->__statep->__value.__wchb[cnt] = *(*inptrp)++; >> + for (cnt = 0; cnt < cnt_after; ++cnt) >> + data->__statep->__value.__wchb[cnt] = (*inptrp)[cnt]; >> + *inptrp = inend; >> data->__statep->__count &= ~7; >> data->__statep->__count |= cnt; >> # endif >> > > > Thanks for working on this! I also noticed this same issue on power. > This patch indeed fixes it there as well. > > As for the patch, I'm not that familiar with iconv code, but by checking > the modified snippet, the loops seem equivalent and the pointers are > properly modified as before. So it's mostly harmless, basically > rewriting those lines in a different way to please GCC. > > LGTM. > > -- > Matheus Castanho > @Carlos: Is this patch okay to commit before the release? Bye. Stefan
On 7/6/20 10:30 AM, Stefan Liebler wrote: > On 6/26/20 8:00 PM, Matheus Castanho wrote: >> Hi Stefan, >> >> On 6/16/20 9:24 AM, Stefan Liebler via Libc-alpha wrote: >>> On s390x, I've recognize various -Werror=stringop-overflow messages >>> in iconv/loop.c and iconv/skeleton.c if build with gcc10 -O3. >>> >>> With this commit gcc knows the size and do not raise those errors anymore. >>> --- >>> iconv/loop.c | 14 ++++++++------ >>> iconv/skeleton.c | 8 +++++--- >>> 2 files changed, 13 insertions(+), 9 deletions(-) >>> >>> diff --git a/iconv/loop.c b/iconv/loop.c >>> index 9f7570d591..b032fcd9ac 100644 >>> --- a/iconv/loop.c >>> +++ b/iconv/loop.c >>> @@ -420,8 +420,10 @@ SINGLE(LOOPFCT) (struct __gconv_step *step, >>> # else >>> /* We don't have enough input for another complete input >>> character. */ >>> - while (inptr < inend) >>> - state->__value.__wchb[inlen++] = *inptr++; >>> + size_t inlen_after = inlen + (inend - inptr); >>> + assert (inlen_after <= sizeof (state->__value.__wchb)); >>> + for (; inlen < inlen_after; inlen++) >>> + state->__value.__wchb[inlen] = *inptr++; >>> # endif >>> >>> return __GCONV_INCOMPLETE_INPUT; >>> @@ -483,11 +485,11 @@ SINGLE(LOOPFCT) (struct __gconv_step *step, >>> /* We don't have enough input for another complete input >>> character. */ >>> assert (inend - inptr > (state->__count & ~7)); >>> - assert (inend - inptr <= sizeof (state->__value)); >>> + assert (inend - inptr <= sizeof (state->__value.__wchb)); >>> state->__count = (state->__count & ~7) | (inend - inptr); >>> - inlen = 0; >>> - while (inptr < inend) >>> - state->__value.__wchb[inlen++] = *inptr++; >>> + for (inlen = 0; inlen < inend - inptr; inlen++) >>> + state->__value.__wchb[inlen] = inptr[inlen]; >>> + inptr = inend; >>> # endif >>> } >>> >>> diff --git a/iconv/skeleton.c b/iconv/skeleton.c >>> index 1dc642e2fc..1a38b51a5a 100644 >>> --- a/iconv/skeleton.c >>> +++ b/iconv/skeleton.c >>> @@ -795,11 +795,13 @@ FUNCTION_NAME (struct __gconv_step *step, struct __gconv_step_data *data, >>> # else >>> /* Make sure the remaining bytes fit into the state objects >>> buffer. */ >>> - assert (inend - *inptrp < 4); >>> + size_t cnt_after = inend - *inptrp; >>> + assert (cnt_after <= sizeof (data->__statep->__value.__wchb)); >>> >>> size_t cnt; >>> - for (cnt = 0; *inptrp < inend; ++cnt) >>> - data->__statep->__value.__wchb[cnt] = *(*inptrp)++; >>> + for (cnt = 0; cnt < cnt_after; ++cnt) >>> + data->__statep->__value.__wchb[cnt] = (*inptrp)[cnt]; >>> + *inptrp = inend; >>> data->__statep->__count &= ~7; >>> data->__statep->__count |= cnt; >>> # endif >>> >> >> >> Thanks for working on this! I also noticed this same issue on power. >> This patch indeed fixes it there as well. >> >> As for the patch, I'm not that familiar with iconv code, but by checking >> the modified snippet, the loops seem equivalent and the pointers are >> properly modified as before. So it's mostly harmless, basically >> rewriting those lines in a different way to please GCC. >> >> LGTM. >> >> -- >> Matheus Castanho >> > > @Carlos: Is this patch okay to commit before the release? Yes, this doesn't change ABI/API and fixes compiles with gcc 10. It is not subject to the ABI/API freeze currently in effect. Please do continue to fix bugs and enable operation with the latest released upstream toolchains.
On 7/6/20 5:03 PM, Carlos O'Donell wrote: > On 7/6/20 10:30 AM, Stefan Liebler wrote: >> On 6/26/20 8:00 PM, Matheus Castanho wrote: >>> Hi Stefan, >>> >>> On 6/16/20 9:24 AM, Stefan Liebler via Libc-alpha wrote: >>>> On s390x, I've recognize various -Werror=stringop-overflow messages >>>> in iconv/loop.c and iconv/skeleton.c if build with gcc10 -O3. >>>> >>>> With this commit gcc knows the size and do not raise those errors anymore. ... >>>> >>> >>> >>> Thanks for working on this! I also noticed this same issue on power. >>> This patch indeed fixes it there as well. >>> >>> As for the patch, I'm not that familiar with iconv code, but by checking >>> the modified snippet, the loops seem equivalent and the pointers are >>> properly modified as before. So it's mostly harmless, basically >>> rewriting those lines in a different way to please GCC. >>> >>> LGTM. >>> >>> -- >>> Matheus Castanho >>> >> >> @Carlos: Is this patch okay to commit before the release? > > Yes, this doesn't change ABI/API and fixes compiles with gcc 10. > > It is not subject to the ABI/API freeze currently in effect. > > Please do continue to fix bugs and enable operation with the > latest released upstream toolchains. > Committed. Thanks. Stefan
diff --git a/iconv/loop.c b/iconv/loop.c index 9f7570d591..b032fcd9ac 100644 --- a/iconv/loop.c +++ b/iconv/loop.c @@ -420,8 +420,10 @@ SINGLE(LOOPFCT) (struct __gconv_step *step, # else /* We don't have enough input for another complete input character. */ - while (inptr < inend) - state->__value.__wchb[inlen++] = *inptr++; + size_t inlen_after = inlen + (inend - inptr); + assert (inlen_after <= sizeof (state->__value.__wchb)); + for (; inlen < inlen_after; inlen++) + state->__value.__wchb[inlen] = *inptr++; # endif return __GCONV_INCOMPLETE_INPUT; @@ -483,11 +485,11 @@ SINGLE(LOOPFCT) (struct __gconv_step *step, /* We don't have enough input for another complete input character. */ assert (inend - inptr > (state->__count & ~7)); - assert (inend - inptr <= sizeof (state->__value)); + assert (inend - inptr <= sizeof (state->__value.__wchb)); state->__count = (state->__count & ~7) | (inend - inptr); - inlen = 0; - while (inptr < inend) - state->__value.__wchb[inlen++] = *inptr++; + for (inlen = 0; inlen < inend - inptr; inlen++) + state->__value.__wchb[inlen] = inptr[inlen]; + inptr = inend; # endif } diff --git a/iconv/skeleton.c b/iconv/skeleton.c index 1dc642e2fc..1a38b51a5a 100644 --- a/iconv/skeleton.c +++ b/iconv/skeleton.c @@ -795,11 +795,13 @@ FUNCTION_NAME (struct __gconv_step *step, struct __gconv_step_data *data, # else /* Make sure the remaining bytes fit into the state objects buffer. */ - assert (inend - *inptrp < 4); + size_t cnt_after = inend - *inptrp; + assert (cnt_after <= sizeof (data->__statep->__value.__wchb)); size_t cnt; - for (cnt = 0; *inptrp < inend; ++cnt) - data->__statep->__value.__wchb[cnt] = *(*inptrp)++; + for (cnt = 0; cnt < cnt_after; ++cnt) + data->__statep->__value.__wchb[cnt] = (*inptrp)[cnt]; + *inptrp = inend; data->__statep->__count &= ~7; data->__statep->__count |= cnt; # endif