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

["Terry Guo" <terry dot guo at arm dot com>]

> -----Original Message-----
> From: newlib-owner@sourceware.org [mailto:newlib-
> owner@sourceware.org] On Behalf Of Bin.Cheng
> Sent: Wednesday, November 05, 2014 6:07 PM
> To: newlib@sourceware.org; Bin Cheng
> Subject: Re: [Patch, Newlib-nano]Improve printf to support non nul-
> terminated string
> 
> 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

Thanks for comments. Patch is updated per your suggestion. Is it OK?

BR,
Terry

Attachment: nano-non-nul-terminated-v4.txt
Description: Text document