Re: [Patch, Newlib-nano]Improve printf to support non nul-terminated string

["Bin.Cheng" <amker dot cheng at gmail dot com>]

On Wed, Nov 5, 2014 at 5:15 PM, Corinna Vinschen <vinschen@redhat.com> wrote:
> Hi Bin,
>
> since that's your code, can you please take a look into this patch?
>
>
> Thanks,
> Corinna
>
>
> On Nov  1 11:16, Terry Guo wrote:
>> Hi there,
>>
>> When use printf to print string that isn't nul-terminated like below:
>>
>> char cp[16];
>> printf("%.*s", sizeof(cp), moo);
>>
>> current printf implementation in newlib-nano has a subtle bug that is the
>> call to strlen(cp). Suppose all 16 elements in cp are non-nul characters and
>> the storage next to cp is filled with value 0xFF (which is quite common for
>> flash of an embedded device), the strlen function will travel the memory
>> until trigger an error.
>>
>> The Newlib implementation already realized this issue. The attached patch
>> intends to reuse Newlib implementation here for Newlib-nano to avoid such
>> issue.
>>
>> BR,
>> Terry
>>
>> 2014-11-01  Terry Guo  <terry.guo@arm.com>
>>
>>      * libc/stdio/nano-vfprintf_i.c (_printf_i): Use Newlib approach to
>>      handle string that might be not nul-terminated.
>>      * testsuite/newlib.stdio/nulprintf.c: New test.
>
>> diff --git a/newlib/libc/stdio/nano-vfprintf_i.c b/newlib/libc/stdio/nano-vfprintf_i.c
>> index b1b0d1d..2bc459d 100644
>> --- a/newlib/libc/stdio/nano-vfprintf_i.c
>> +++ b/newlib/libc/stdio/nano-vfprintf_i.c
>> @@ -211,8 +211,14 @@ number:
>>      case 's':
>>        cp = GET_ARG (N, *ap, char_ptr_t);
>>        /* Precision gives the maximum number of chars to be written from a
>> -      string, and take prec == -1 into consideration.  */
>> -      if ((u_int)(pdata->size = strlen (cp)) > (u_int)(pdata->prec))
>> +      string, and take prec == -1 into consideration.
>> +      Use normal Newlib approach here to support case where cp is not
>> +      nul-terminated.  */
>> +      char *p = memchr (cp, 0, pdata->prec);
>> +
>> +      if (p != NULL)
>> +     pdata->size = p - cp;
>> +      else
>>       pdata->size = pdata->prec;
>>        /* Below code is kept for reading.  The check is redundant because
>>        pdata->prec will be set to pdata->size if it is -1 previously.  */
>> diff --git a/newlib/testsuite/newlib.stdio/nulprintf.c b/newlib/testsuite/newlib.stdio/nulprintf.c
>> new file mode 100644
>> index 0000000..5e4131b
>> --- /dev/null
>> +++ b/newlib/testsuite/newlib.stdio/nulprintf.c
>> @@ -0,0 +1,17 @@
>> +/*
>> + * Copyright (C) 2014 by ARM Ltd. All rights reserved.
>> + *
>> + * Permission to use, copy, modify, and distribute this software
>> + * is freely granted, provided that this notice is preserved.
>> + */
>> +
>> +#include <stdio.h>
>> +#include "check.h"
>> +
>> +const char m[8] = {'M','M','M','M','M','M','M','M'};
>> +
>> +int main()
>> +{
>> +  printf ("%.*s\n", 8, m);
>> +  exit (0);
>> +}
>
>
> --
> Corinna Vinschen
> Cygwin Maintainer
> Red Hat

Hi,
Thanks for fixing the issue.  Only thing is that code snippet after change:

      char *p = memchr (cp, 0, pdata->prec);

      if (p != NULL)
    pdata->size = p - cp;
      else
    pdata->size = pdata->prec;
      /* Below code is kept for reading.  The check is redundant because
     pdata->prec will be set to pdata->size if it is -1 previously.  */
#if 0
      if (pdata->prec > pdata->size)
#endif
      pdata->prec = pdata->size;
      goto non_number_nosign;

Equals to below form, which is of smaller size:

      char *p = memchr (cp, 0, pdata->prec);

      if (p != NULL)
    pdata->prec = p - cp;

      pdata->size = pdata->prec;
      goto non_number_nosign;

Does it?

Hi Corinna,
Have to leave test case part to you since I am not familiar with
newlib test infrastructure.

Thanks,
bin