Re: [PATCH v3] Fix off-by-one OOB write in iconv/tst-iconv-mt

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

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);
>  }