Xen devel - Nov 2011 - [PATCH] Convert hvmloader sprintf() into snprintf()

# HG changeset patch
# User Paul Durrant <paul.durrant@citrix.com>
# Date 1322574114 0
# Node ID e1e952982cf1d7a0c38a7822a8b5e78ba04b5ba5
# Parent  225da1242ba979ddc8c48767d3822e0c8d274ae1
Convert hvmloader sprintf() into snprintf().

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>

diff -r 225da1242ba9 -r e1e952982cf1 tools/firmware/hvmloader/acpi/build.c
--- a/tools/firmware/hvmloader/acpi/build.c	Tue Nov 29 10:48:54 2011 +0000
+++ b/tools/firmware/hvmloader/acpi/build.c	Tue Nov 29 13:41:54 2011 +0000
@@ -306,7 +306,8 @@ unsigned long new_vm_gid(void)
     buf = mem_alloc(8, 8);
     if (!buf) return 0;
 
-    sprintf(addr, "0x%lx", virt_to_phys(buf));
+    if (snprintf(addr, 11, "0x%lx", virt_to_phys(buf)) >= 11)
return 0;
+
     xenstore_write("data/generation-id", addr);
 
     gid = strtoll(xenstore_read("platform/generation-id",
"0"), NULL, 0);
diff -r 225da1242ba9 -r e1e952982cf1 tools/firmware/hvmloader/util.c
--- a/tools/firmware/hvmloader/util.c	Tue Nov 29 10:48:54 2011 +0000
+++ b/tools/firmware/hvmloader/util.c	Tue Nov 29 13:41:54 2011 +0000
@@ -528,7 +528,7 @@ static char *printnum(char *p, unsigned 
     return p;
 }
 
-static void _doprint(void (*emit)(char**, char), char **arg, const char *fmt,
va_list ap)
+static void _doprint(void (*emit)(void *, char), void *arg, const char *fmt,
va_list ap)
 {
     char *str, c;
     int lflag, zflag, nflag;
@@ -626,7 +626,7 @@ static void putchar(char c)
     outb(0xe9, c);
 }
 
-static void __put(char **ignore, char c)
+static void __put(void *arg, char c)
 {
     putchar(c);
 }
@@ -648,22 +648,42 @@ int vprintf(const char *fmt, va_list ap)
     return 0;
 }
 
-static void __copy(char **buf, char c)
+struct __copy_context {
+    char *ptr;
+    size_t emitted;
+    size_t remaining;
+};
+
+static void __copy(void *arg, char c)
 {
-    **buf = c;
-    (*buf)++;
+    struct __copy_context *ctxt = arg;
+
+    ctxt->emitted++;
+
+    if (ctxt->remaining == 0)
+        return;
+    
+    *(ctxt->ptr++) = c;
+    --ctxt->remaining;
 }
 
-int sprintf(char *buf, const char *fmt, ...)
+int snprintf(char *buf, size_t size, const char *fmt, ...)
 {
     va_list ap;
+    struct __copy_context ctxt;
+
+    ctxt.ptr = buf;
+    ctxt.emitted = 0;
+    ctxt.remaining = size;
 
     va_start(ap, fmt);
-    _doprint(__copy, &buf, fmt, ap);
+    _doprint(__copy, &ctxt, fmt, ap);
     va_end(ap);
 
-    *buf = ''\0'';
-    return 0;
+    if (ctxt.remaining != 0)
+        *ctxt.ptr = ''\0'';
+
+    return ctxt.emitted;
 }
 
 static void __attribute__((noreturn)) crash(void)
diff -r 225da1242ba9 -r e1e952982cf1 tools/firmware/hvmloader/util.h
--- a/tools/firmware/hvmloader/util.h	Tue Nov 29 10:48:54 2011 +0000
+++ b/tools/firmware/hvmloader/util.h	Tue Nov 29 13:41:54 2011 +0000
@@ -172,7 +172,8 @@ int printf(const char *fmt, ...) __attri
 int vprintf(const char *fmt, va_list ap);
 
 /* Buffer output */
-int sprintf(char *buf, const char *fmt, ...) __attribute__ ((format (printf, 2,
3)));
+typedef unsigned long size_t;
+int snprintf(char *buf, size_t size, const char *fmt, ...) __attribute__
((format (printf, 3, 4)));
 
 /* Populate specified memory hole with RAM. */
 void mem_hole_populate_ram(xen_pfn_t mfn, uint32_t nr_mfns);

On 29/11/11 13:42, Paul Durrant wrote:
> # HG changeset patch
> # User Paul Durrant <paul.durrant@citrix.com>
> # Date 1322574114 0
> # Node ID e1e952982cf1d7a0c38a7822a8b5e78ba04b5ba5
> # Parent  225da1242ba979ddc8c48767d3822e0c8d274ae1
> Convert hvmloader sprintf() into snprintf().
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> 
> diff -r 225da1242ba9 -r e1e952982cf1 tools/firmware/hvmloader/acpi/build.c
> --- a/tools/firmware/hvmloader/acpi/build.c	Tue Nov 29 10:48:54 2011 +0000
> +++ b/tools/firmware/hvmloader/acpi/build.c	Tue Nov 29 13:41:54 2011 +0000
> @@ -306,7 +306,8 @@ unsigned long new_vm_gid(void)
>      buf = mem_alloc(8, 8);
>      if (!buf) return 0;
>  
> -    sprintf(addr, "0x%lx", virt_to_phys(buf));
> +    if (snprintf(addr, 11, "0x%lx", virt_to_phys(buf)) >= 11)
return 0;
> +
>      xenstore_write("data/generation-id", addr);
>  
>      gid = strtoll(xenstore_read("platform/generation-id",
"0"), NULL, 0);
> diff -r 225da1242ba9 -r e1e952982cf1 tools/firmware/hvmloader/util.c
> --- a/tools/firmware/hvmloader/util.c	Tue Nov 29 10:48:54 2011 +0000
> +++ b/tools/firmware/hvmloader/util.c	Tue Nov 29 13:41:54 2011 +0000
> @@ -528,7 +528,7 @@ static char *printnum(char *p, unsigned 
>      return p;
>  }
>  
> -static void _doprint(void (*emit)(char**, char), char **arg, const char
*fmt, va_list ap)
> +static void _doprint(void (*emit)(void *, char), void *arg, const char
*fmt, va_list ap)
>  {
>      char *str, c;
>      int lflag, zflag, nflag;
> @@ -626,7 +626,7 @@ static void putchar(char c)
>      outb(0xe9, c);
>  }
>  
> -static void __put(char **ignore, char c)
> +static void __put(void *arg, char c)
>  {
>      putchar(c);
>  }
> @@ -648,22 +648,42 @@ int vprintf(const char *fmt, va_list ap)
>      return 0;
>  }
>  
> -static void __copy(char **buf, char c)
> +struct __copy_context {
> +    char *ptr;
> +    size_t emitted;
> +    size_t remaining;
> +};
> +
> +static void __copy(void *arg, char c)
>  {
> -    **buf = c;
> -    (*buf)++;
> +    struct __copy_context *ctxt = arg;
> +
> +    ctxt->emitted++;
> +
> +    if (ctxt->remaining == 0)
> +        return;
> +    
> +    *(ctxt->ptr++) = c;
> +    --ctxt->remaining;
>  }
>  
> -int sprintf(char *buf, const char *fmt, ...)
> +int snprintf(char *buf, size_t size, const char *fmt, ...)
>  {
>      va_list ap;
> +    struct __copy_context ctxt;
> +
> +    ctxt.ptr = buf;
> +    ctxt.emitted = 0;
> +    ctxt.remaining = size;
>  
>      va_start(ap, fmt);
> -    _doprint(__copy, &buf, fmt, ap);
> +    _doprint(__copy, &ctxt, fmt, ap);
>      va_end(ap);
>  
> -    *buf = ''\0'';
> -    return 0;
> +    if (ctxt.remaining != 0)
> +        *ctxt.ptr = ''\0'';
> +
> +    return ctxt.emitted;
>  }
This doesn''t return the correct value according the C99.  From the
snprintf() man page:

"The functions snprintf() and vsnprintf() do not write  more  than  size
 bytes  (including  the trailing ''\0'').  If the output was
truncated due
 to this limit then the return value is the number  of  characters  (not
 including the trailing ''\0'') which would have been written to
the final
 string if enough space had been available.  Thus,  a  return  value  of
 size  or  more  means  that  the output was truncated."

David

> 
> This doesn''t return the correct value according the C99.  From the
> snprintf() man page:
> 
> "The functions snprintf() and vsnprintf() do not write  more  than
> size  bytes  (including  the trailing ''\0'').  If the
output was
> truncated due  to this limit then the return value is the number  of
> characters  (not  including the trailing ''\0'') which
would have been
> written to the final  string if enough space had been available.
> Thus,  a  return  value  of  size  or  more  means  that  the output
> was truncated."
> 
...and that matters because? I didn''t say anywhere that the
implementation was C99 compliant.

  Paul

On 29/11/11 14:03, Paul Durrant wrote:
>>
>> This doesn''t return the correct value according the C99.  From
the
>> snprintf() man page:
>>
>> "The functions snprintf() and vsnprintf() do not write  more  than
>> size  bytes  (including  the trailing ''\0'').  If the
output was
>> truncated due  to this limit then the return value is the number  of
>> characters  (not  including the trailing ''\0'') which
would have been
>> written to the final  string if enough space had been available.
>> Thus,  a  return  value  of  size  or  more  means  that  the output
>> was truncated."
>>
> 
> ...and that matters because? I didn''t say anywhere that the
implementation was C99 compliant.
I suggest giving it a different name then.

David

> -----Original Message-----
> From: David Vrabel
> Sent: 29 November 2011 13:57
> To: Paul Durrant
> Cc: xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH] Convert hvmloader sprintf() into
> snprintf()
> 
[snip]
> > +static void __copy(void *arg, char c)
> >  {
> > -    **buf = c;
> > -    (*buf)++;
> > +    struct __copy_context *ctxt = arg;
> > +
> > +    ctxt->emitted++;
> > +
> > +    if (ctxt->remaining == 0)
> > +        return;
> > +
> > +    *(ctxt->ptr++) = c;
> > +    --ctxt->remaining;
> >  }
> >
> > -int sprintf(char *buf, const char *fmt, ...)
> > +int snprintf(char *buf, size_t size, const char *fmt, ...)
> >  {
> >      va_list ap;
> > +    struct __copy_context ctxt;
> > +
> > +    ctxt.ptr = buf;
> > +    ctxt.emitted = 0;
> > +    ctxt.remaining = size;
> >
> >      va_start(ap, fmt);
> > -    _doprint(__copy, &buf, fmt, ap);
> > +    _doprint(__copy, &ctxt, fmt, ap);
> >      va_end(ap);
> >
> > -    *buf = ''\0'';
> > -    return 0;
> > +    if (ctxt.remaining != 0)
> > +        *ctxt.ptr = ''\0'';
> > +
> > +    return ctxt.emitted;
> >  }
> 
> This doesn''t return the correct value according the C99.  From the
> snprintf() man page:
> 
> "The functions snprintf() and vsnprintf() do not write  more  than
> size  bytes  (including  the trailing ''\0'').  If the
output was
> truncated due  to this limit then the return value is the number  of
> characters  (not  including the trailing ''\0'') which
would have been
> written to the final  string if enough space had been available.
> Thus,  a  return  value  of  size  or  more  means  that  the output
> was truncated."
> 
Actually, reading the code again, it is correct isn''t it? ctxt.emitted
is bumped for every character emitted by _doprint() regardless of whether it
makes it into the buffer or not so in an overflow case the value returned will
be the number of characters which would have been written not including the nul
terminator.

  Paul

> -----Original Message-----
> From: David Vrabel
[snip]
> >
> > ...and that matters because? I didn''t say anywhere that the
> implementation was C99 compliant.
> 
> I suggest giving it a different name then.
> 
Should we change the name of printf() and vprintf() too then?

  Paul

On 29/11/11 14:12, Paul Durrant wrote:
>> -----Original Message-----
>> From: David Vrabel
>> Sent: 29 November 2011 13:57
>> To: Paul Durrant
>> Cc: xen-devel@lists.xensource.com
>> Subject: Re: [Xen-devel] [PATCH] Convert hvmloader sprintf() into
>> snprintf()
>>
> [snip]
>>> +static void __copy(void *arg, char c)
>>>  {
>>> -    **buf = c;
>>> -    (*buf)++;
>>> +    struct __copy_context *ctxt = arg;
>>> +
>>> +    ctxt->emitted++;
>>> +
>>> +    if (ctxt->remaining == 0)
>>> +        return;
>>> +
>>> +    *(ctxt->ptr++) = c;
>>> +    --ctxt->remaining;
>>>  }
>>>
>>> -int sprintf(char *buf, const char *fmt, ...)
>>> +int snprintf(char *buf, size_t size, const char *fmt, ...)
>>>  {
>>>      va_list ap;
>>> +    struct __copy_context ctxt;
>>> +
>>> +    ctxt.ptr = buf;
>>> +    ctxt.emitted = 0;
>>> +    ctxt.remaining = size;
>>>
>>>      va_start(ap, fmt);
>>> -    _doprint(__copy, &buf, fmt, ap);
>>> +    _doprint(__copy, &ctxt, fmt, ap);
>>>      va_end(ap);
>>>
>>> -    *buf = ''\0'';
>>> -    return 0;
>>> +    if (ctxt.remaining != 0)
>>> +        *ctxt.ptr = ''\0'';
>>> +
>>> +    return ctxt.emitted;
>>>  }
>>
>> This doesn''t return the correct value according the C99.  From
the
>> snprintf() man page:
>>
>> "The functions snprintf() and vsnprintf() do not write  more  than
>> size  bytes  (including  the trailing ''\0'').  If the
output was
>> truncated due  to this limit then the return value is the number  of
>> characters  (not  including the trailing ''\0'') which
would have been
>> written to the final  string if enough space had been available.
>> Thus,  a  return  value  of  size  or  more  means  that  the output
>> was truncated."
>>
>
> Actually, reading the code again, it is correct isn''t it?
> ctxt.emitted is bumped for every character emitted by _doprint()
> regardless of whether it makes it into the buffer or not so in an
> overflow case the value returned will be the number of characters
> which would have been written not including the nul terminator.
Er. Yes, it is correct. My mistake.

David