| Message ID | 20220603125251.3903158-1-stli@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Avoid -Wstringop-overflow= warning in iconv module. | expand |
On 03/06/2022 18:22, Stefan Liebler via Libc-alpha wrote: > On s390x when compiling with GCC 12, I get this warning: > utf8-utf16-z9.c: > ../iconv/loop.c: In function ‘__from_utf8_loop_etf3eh_single’: > ../iconv/loop.c:445:22: error: writing 1 byte into a region of size 0 [-Werror=stringop-overflow=] > 445 | bytebuf[inlen++] = *inptr++; > | ~~~~~~~~~~~~~~~~~^~~~~~~~~~ > ../iconv/loop.c:381:17: note: at offset 4 into destination object ‘bytebuf’ of size 4 > 381 | unsigned char bytebuf[MAX_NEEDED_INPUT]; > | ^~~~~~~ > ../iconv/loop.c:445:22: error: writing 1 byte into a region of size 0 [-Werror=stringop-overflow=] > 445 | bytebuf[inlen++] = *inptr++; > | ~~~~~~~~~~~~~~~~~^~~~~~~~~~ > ../iconv/loop.c:381:17: note: at offset 5 into destination object ‘bytebuf’ of size 4 > 381 | unsigned char bytebuf[MAX_NEEDED_INPUT]; > | ^~~~~~~ > > This patch tells the compiler that inend is always behind inptr which > avoids the warning. Note that the SINGLE function is only used to > implement the mb*towc*() or wc*tomb*() functions. Those functions use > inptr and inend pointing to a variable on stack, compute the inend pointer > or explicitly check the arguments which always leads to inptr < inend. LGTM; there was a time that this wasn't true for applications that assumed POSIX behaviour in wcrtomb but it is true now. See also: commit 9bcd12d223a8990254b65e2dada54faa5d2742f3 Author: Siddhesh Poyarekar <siddhesh@sourceware.org> Date: Fri May 13 19:10:15 2022 +0530 wcrtomb: Make behavior POSIX compliant Please add a note in the git commit log that if someone wants to backport this patch to release branches, they should also backport this wcrtomb change. Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org> > --- > iconv/loop.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/iconv/loop.c b/iconv/loop.c > index f8727a637a..09ade3b765 100644 > --- a/iconv/loop.c > +++ b/iconv/loop.c > @@ -435,11 +435,17 @@ SINGLE(LOOPFCT) (struct __gconv_step *step, > return __GCONV_FULL_OUTPUT; > > /* Now add characters from the normal input buffer. */ > - if (inlen >= MAX_NEEDED_INPUT) > + if (inlen >= MAX_NEEDED_INPUT || inptr >= inend) > /* Avoid a -Wstringop-overflow= warning when this loop is > unrolled. The compiler cannot otherwise see that this is > unreachable because it depends on (state->__count & 7) not > - being too large after a previous conversion step. */ > + being too large after a previous conversion step. > + Starting with GCC 12, we also have mark the inptr >= inend > + case as unreachable to omit the warning. Note that this SINGLE > + function is only used to implement the mb*towc*() or wc*tomb*() > + functions. Those functions use inptr and inend pointing to a > + variable on stack, compute the inend pointer or explicitly check > + the arguments which always leads to inptr < inend. */ > __builtin_unreachable (); > do > bytebuf[inlen++] = *inptr++;
On 13/06/2022 08:31, Siddhesh Poyarekar wrote: > On 03/06/2022 18:22, Stefan Liebler via Libc-alpha wrote: >> On s390x when compiling with GCC 12, I get this warning: >> utf8-utf16-z9.c: >> ../iconv/loop.c: In function ‘__from_utf8_loop_etf3eh_single’: >> ../iconv/loop.c:445:22: error: writing 1 byte into a region of size 0 >> [-Werror=stringop-overflow=] >> 445 | bytebuf[inlen++] = *inptr++; >> | ~~~~~~~~~~~~~~~~~^~~~~~~~~~ >> ../iconv/loop.c:381:17: note: at offset 4 into destination object >> ‘bytebuf’ of size 4 >> 381 | unsigned char bytebuf[MAX_NEEDED_INPUT]; >> | ^~~~~~~ >> ../iconv/loop.c:445:22: error: writing 1 byte into a region of size 0 >> [-Werror=stringop-overflow=] >> 445 | bytebuf[inlen++] = *inptr++; >> | ~~~~~~~~~~~~~~~~~^~~~~~~~~~ >> ../iconv/loop.c:381:17: note: at offset 5 into destination object >> ‘bytebuf’ of size 4 >> 381 | unsigned char bytebuf[MAX_NEEDED_INPUT]; >> | ^~~~~~~ >> >> This patch tells the compiler that inend is always behind inptr which >> avoids the warning. Note that the SINGLE function is only used to >> implement the mb*towc*() or wc*tomb*() functions. Those functions use >> inptr and inend pointing to a variable on stack, compute the inend >> pointer >> or explicitly check the arguments which always leads to inptr < inend. > > LGTM; there was a time that this wasn't true for applications that > assumed POSIX behaviour in wcrtomb but it is true now. See also: > > commit 9bcd12d223a8990254b65e2dada54faa5d2742f3 > Author: Siddhesh Poyarekar <siddhesh@sourceware.org> > Date: Fri May 13 19:10:15 2022 +0530 > > wcrtomb: Make behavior POSIX compliant > > Please add a note in the git commit log that if someone wants to > backport this patch to release branches, they should also backport this > wcrtomb change. > > Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org> > Hi Siddhesh, thanks for reviewing and the note regarding wcrtomb. Of course I've added such a note and committed the patch. Thanks, Stefan
diff --git a/iconv/loop.c b/iconv/loop.c index f8727a637a..09ade3b765 100644 --- a/iconv/loop.c +++ b/iconv/loop.c @@ -435,11 +435,17 @@ SINGLE(LOOPFCT) (struct __gconv_step *step, return __GCONV_FULL_OUTPUT; /* Now add characters from the normal input buffer. */ - if (inlen >= MAX_NEEDED_INPUT) + if (inlen >= MAX_NEEDED_INPUT || inptr >= inend) /* Avoid a -Wstringop-overflow= warning when this loop is unrolled. The compiler cannot otherwise see that this is unreachable because it depends on (state->__count & 7) not - being too large after a previous conversion step. */ + being too large after a previous conversion step. + Starting with GCC 12, we also have mark the inptr >= inend + case as unreachable to omit the warning. Note that this SINGLE + function is only used to implement the mb*towc*() or wc*tomb*() + functions. Those functions use inptr and inend pointing to a + variable on stack, compute the inend pointer or explicitly check + the arguments which always leads to inptr < inend. */ __builtin_unreachable (); do bytebuf[inlen++] = *inptr++;
On s390x when compiling with GCC 12, I get this warning: utf8-utf16-z9.c: ../iconv/loop.c: In function ‘__from_utf8_loop_etf3eh_single’: ../iconv/loop.c:445:22: error: writing 1 byte into a region of size 0 [-Werror=stringop-overflow=] 445 | bytebuf[inlen++] = *inptr++; | ~~~~~~~~~~~~~~~~~^~~~~~~~~~ ../iconv/loop.c:381:17: note: at offset 4 into destination object ‘bytebuf’ of size 4 381 | unsigned char bytebuf[MAX_NEEDED_INPUT]; | ^~~~~~~ ../iconv/loop.c:445:22: error: writing 1 byte into a region of size 0 [-Werror=stringop-overflow=] 445 | bytebuf[inlen++] = *inptr++; | ~~~~~~~~~~~~~~~~~^~~~~~~~~~ ../iconv/loop.c:381:17: note: at offset 5 into destination object ‘bytebuf’ of size 4 381 | unsigned char bytebuf[MAX_NEEDED_INPUT]; | ^~~~~~~ This patch tells the compiler that inend is always behind inptr which avoids the warning. Note that the SINGLE function is only used to implement the mb*towc*() or wc*tomb*() functions. Those functions use inptr and inend pointing to a variable on stack, compute the inend pointer or explicitly check the arguments which always leads to inptr < inend. --- iconv/loop.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)