Message ID | 20231002141631.1882760-1-szabolcs.nagy@arm.com |
---|---|
State | New |
Headers | show |
Series | [v3] Fix off-by-one OOB write in iconv/tst-iconv-mt | expand |
On 02/10/23 11:16, Szabolcs Nagy wrote: > The iconv buffer sizes must not include the \0 string terminator. > And the output termination with *outbufpos = '\0' was OOB. > > Consistently use non-null-terminated buffer sizes. LGTM, valgrind does show the issue: ==3486612== Thread 9: ==3486612== Invalid write of size 1 ==3486612== at 0x10A78F: worker (tst-iconv-mt.c:94) ==3486612== by 0x48E3FCB: start_thread (pthread_create.c:444) ==3486612== by 0x496DFFF: clone (clone.S:100) ==3486612== Address 0x52579ae is 0 bytes after a block of size 14 alloc'd ==3486612== at 0x4845A83: calloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so) ==3486612== by 0x10B6E0: xcalloc (xcalloc.c:30) ==3486612== by 0x10A70A: worker (tst-iconv-mt.c:63) ==3486612== by 0x48E3FCB: start_thread (pthread_create.c:444) ==3486612== by 0x496DFFF: clone (clone.S:100) Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> > > --- > v2: dropped \0 and replaced strncmp with TEST_COMPARE_BLOB. > v3: unchanged. (rebase) > --- > iconv/tst-iconv-mt.c | 15 +++++---------- > 1 file changed, 5 insertions(+), 10 deletions(-) > > diff --git a/iconv/tst-iconv-mt.c b/iconv/tst-iconv-mt.c > index e634eec1b7..8d7867b323 100644 > --- a/iconv/tst-iconv-mt.c > +++ b/iconv/tst-iconv-mt.c > @@ -57,12 +57,13 @@ worker (void * arg) > iconv_t cd; > > char ascii[] = CONV_INPUT; > + size_t bytes = sizeof (CONV_INPUT) - 1; > char *inbufpos = ascii; > - size_t inbytesleft = sizeof (CONV_INPUT); > + size_t inbytesleft = bytes; > > - char *utf8 = xcalloc (sizeof (CONV_INPUT), 1); > + char *utf8 = xcalloc (bytes, 1); > char *outbufpos = utf8; > - size_t outbytesleft = sizeof (CONV_INPUT); > + size_t outbytesleft = bytes; > > if (tidx < TCOUNT/2) > /* The first half of the worker thread pool synchronize together here, > @@ -91,8 +92,6 @@ worker (void * arg) > &outbytesleft) > != (size_t) -1); > > - *outbufpos = '\0'; > - > xpthread_barrier_wait (&sync); > > TEST_VERIFY_EXIT (iconv_close (cd) == 0); > @@ -104,11 +103,7 @@ worker (void * arg) > if (tidx < TCOUNT/2) > xpthread_barrier_wait (&sync); > > - if (strncmp (utf8, CONV_INPUT, sizeof CONV_INPUT)) > - { > - printf ("FAIL: thread %lx: invalid conversion output from iconv\n", tidx); > - pthread_exit ((void *) (long int) 1); > - } > + TEST_COMPARE_BLOB (utf8, bytes, CONV_INPUT, bytes); > > pthread_exit (NULL); > }
diff --git a/iconv/tst-iconv-mt.c b/iconv/tst-iconv-mt.c index e634eec1b7..8d7867b323 100644 --- a/iconv/tst-iconv-mt.c +++ b/iconv/tst-iconv-mt.c @@ -57,12 +57,13 @@ worker (void * arg) iconv_t cd; char ascii[] = CONV_INPUT; + size_t bytes = sizeof (CONV_INPUT) - 1; char *inbufpos = ascii; - size_t inbytesleft = sizeof (CONV_INPUT); + size_t inbytesleft = bytes; - char *utf8 = xcalloc (sizeof (CONV_INPUT), 1); + char *utf8 = xcalloc (bytes, 1); char *outbufpos = utf8; - size_t outbytesleft = sizeof (CONV_INPUT); + size_t outbytesleft = bytes; if (tidx < TCOUNT/2) /* The first half of the worker thread pool synchronize together here, @@ -91,8 +92,6 @@ worker (void * arg) &outbytesleft) != (size_t) -1); - *outbufpos = '\0'; - xpthread_barrier_wait (&sync); TEST_VERIFY_EXIT (iconv_close (cd) == 0); @@ -104,11 +103,7 @@ worker (void * arg) if (tidx < TCOUNT/2) xpthread_barrier_wait (&sync); - if (strncmp (utf8, CONV_INPUT, sizeof CONV_INPUT)) - { - printf ("FAIL: thread %lx: invalid conversion output from iconv\n", tidx); - pthread_exit ((void *) (long int) 1); - } + TEST_COMPARE_BLOB (utf8, bytes, CONV_INPUT, bytes); pthread_exit (NULL); }