[PATCH] libxc: Fix buffer length for get_suspend_file

[PATCH] libxc: Fix buffer length for get_suspend_file

[Ian Jackson]

Declaring a formal parameter to have an array type doesn't result in
the parameter actually having an array type.  The type is "adjusted"
to a pointer.  (C99 6.9.1(7), 6.7.5.3.)

So the use of sizeof in xc_suspend.c:get_suspend_file was wrong.
Instead, use the #define.  Also get rid of the array parameter, as it
is misleading.

Newer versions of gcc warn about the erroneous code:
  xc_suspend.c:39:25: error: argument to 'sizeof' in 'snprintf' call
    is the same expression as the destination; did you mean to provide
    an explicit length? [-Werror=sizeof-pointer-memaccess]

Reported-By: Julien Grall <julien.grall@linaro.org>
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Julien Grall <julien.grall@linaro.org>
---
 tools/libxc/xc_suspend.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxc/xc_suspend.c b/tools/libxc/xc_suspend.c
index 84ee139..798eaaa 100644
--- a/tools/libxc/xc_suspend.c
+++ b/tools/libxc/xc_suspend.c
@@ -36,7 +36,7 @@
 
 static void get_suspend_file(char buf[SUSPEND_FILE_BUFLEN], int domid)
 {
-    snprintf(buf, sizeof(buf), SUSPEND_LOCK_FILE, domid);
+    snprintf(buf, SUSPEND_FILE_BUFLEN, SUSPEND_LOCK_FILE, domid);
 }
 
 static int lock_suspend_event(xc_interface *xch, int domid, int *lockfd)
-- 
1.7.10.4

Re: [PATCH] libxc: Fix buffer length for get_suspend_file

[Ian Campbell]

On Tue, 2014-03-18 at 13:54 +0000, Ian Jackson wrote:
> Declaring a formal parameter to have an array type doesn't result in
> the parameter actually having an array type.  The type is "adjusted"
> to a pointer.  (C99 6.9.1(7), 6.7.5.3.)
> 
> So the use of sizeof in xc_suspend.c:get_suspend_file was wrong.
> Instead, use the #define.  Also get rid of the array parameter, as it
> is misleading.

Did you actually do this bit?

> Newer versions of gcc warn about the erroneous code:
>   xc_suspend.c:39:25: error: argument to 'sizeof' in 'snprintf' call
>     is the same expression as the destination; did you mean to provide
>     an explicit length? [-Werror=sizeof-pointer-memaccess]
> 
> Reported-By: Julien Grall <julien.grall@linaro.org>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> CC: Julien Grall <julien.grall@linaro.org>
> ---
>  tools/libxc/xc_suspend.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/libxc/xc_suspend.c b/tools/libxc/xc_suspend.c
> index 84ee139..798eaaa 100644
> --- a/tools/libxc/xc_suspend.c
> +++ b/tools/libxc/xc_suspend.c
> @@ -36,7 +36,7 @@
>  
>  static void get_suspend_file(char buf[SUSPEND_FILE_BUFLEN], int domid)
>  {
> -    snprintf(buf, sizeof(buf), SUSPEND_LOCK_FILE, domid);
> +    snprintf(buf, SUSPEND_FILE_BUFLEN, SUSPEND_LOCK_FILE, domid);
>  }
>  
>  static int lock_suspend_event(xc_interface *xch, int domid, int *lockfd)

[PATCH v2] libxc: Fix buffer length for get_suspend_file

[Ian Jackson]

Declaring a formal parameter to have an array type doesn't result in
the parameter actually having an array type.  The type is "adjusted"
to a pointer.  (C99 6.9.1(7), 6.7.5.3.)

So the use of sizeof in xc_suspend.c:get_suspend_file was wrong.
Instead, use the #define.  Also get rid of the array size, as it is
misleading.

Newer versions of gcc warn about the erroneous code:
  xc_suspend.c:39:25: error: argument to 'sizeof' in 'snprintf' call
    is the same expression as the destination; did you mean to provide
    an explicit length? [-Werror=sizeof-pointer-memaccess]

Reported-By: Julien Grall <julien.grall@linaro.org>
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Julien Grall <julien.grall@linaro.org>

--
v2: Actually change the declaration of buf.
---
 tools/libxc/xc_suspend.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libxc/xc_suspend.c b/tools/libxc/xc_suspend.c
index 84ee139..200d381 100644
--- a/tools/libxc/xc_suspend.c
+++ b/tools/libxc/xc_suspend.c
@@ -34,9 +34,9 @@
 
 #define SUSPEND_FILE_BUFLEN (sizeof(SUSPEND_LOCK_FILE) + 10)
 
-static void get_suspend_file(char buf[SUSPEND_FILE_BUFLEN], int domid)
+static void get_suspend_file(char buf[], int domid)
 {
-    snprintf(buf, sizeof(buf), SUSPEND_LOCK_FILE, domid);
+    snprintf(buf, SUSPEND_FILE_BUFLEN, SUSPEND_LOCK_FILE, domid);
 }
 
 static int lock_suspend_event(xc_interface *xch, int domid, int *lockfd)
-- 
1.7.10.4

Re: [PATCH] libxc: Fix buffer length for get_suspend_file

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH] libxc: Fix buffer length for get_suspend_file"):
> On Tue, 2014-03-18 at 13:54 +0000, Ian Jackson wrote:
> > So the use of sizeof in xc_suspend.c:get_suspend_file was wrong.
> > Instead, use the #define.  Also get rid of the array parameter, as it
> > is misleading.
> 
> Did you actually do this bit?

Yes, but apparently I didn't include it in the patch.  See v2.

Thanks,
Ian.

Re: [PATCH v2] libxc: Fix buffer length for get_suspend_file

[Ian Campbell]

On Tue, 2014-03-18 at 13:58 +0000, Ian Jackson wrote:
> Declaring a formal parameter to have an array type doesn't result in
> the parameter actually having an array type.  The type is "adjusted"
> to a pointer.  (C99 6.9.1(7), 6.7.5.3.)
> 
> So the use of sizeof in xc_suspend.c:get_suspend_file was wrong.
> Instead, use the #define.  Also get rid of the array size, as it is
> misleading.
> 
> Newer versions of gcc warn about the erroneous code:
>   xc_suspend.c:39:25: error: argument to 'sizeof' in 'snprintf' call
>     is the same expression as the destination; did you mean to provide
>     an explicit length? [-Werror=sizeof-pointer-memaccess]
> 
> Reported-By: Julien Grall <julien.grall@linaro.org>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Although I do wonder if it would be less error prone to push the size up
to a parameter to get_suspend_file where it is (presumably) near to the
declaration of the array.

> CC: Julien Grall <julien.grall@linaro.org>
> 
> --
> v2: Actually change the declaration of buf.
> ---
>  tools/libxc/xc_suspend.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxc/xc_suspend.c b/tools/libxc/xc_suspend.c
> index 84ee139..200d381 100644
> --- a/tools/libxc/xc_suspend.c
> +++ b/tools/libxc/xc_suspend.c
> @@ -34,9 +34,9 @@
>  
>  #define SUSPEND_FILE_BUFLEN (sizeof(SUSPEND_LOCK_FILE) + 10)
>  
> -static void get_suspend_file(char buf[SUSPEND_FILE_BUFLEN], int domid)
> +static void get_suspend_file(char buf[], int domid)
>  {
> -    snprintf(buf, sizeof(buf), SUSPEND_LOCK_FILE, domid);
> +    snprintf(buf, SUSPEND_FILE_BUFLEN, SUSPEND_LOCK_FILE, domid);
>  }
>  
>  static int lock_suspend_event(xc_interface *xch, int domid, int *lockfd)

Re: [PATCH v2] libxc: Fix buffer length for get_suspend_file

[Julien Grall]

Hi Ian,

On 03/18/2014 01:58 PM, Ian Jackson wrote:
> Declaring a formal parameter to have an array type doesn't result in
> the parameter actually having an array type.  The type is "adjusted"
> to a pointer.  (C99 6.9.1(7), 6.7.5.3.)
> 
> So the use of sizeof in xc_suspend.c:get_suspend_file was wrong.
> Instead, use the #define.  Also get rid of the array size, as it is
> misleading.
> 
> Newer versions of gcc warn about the erroneous code:
>   xc_suspend.c:39:25: error: argument to 'sizeof' in 'snprintf' call
>     is the same expression as the destination; did you mean to provide
>     an explicit length? [-Werror=sizeof-pointer-memaccess]
> 
> Reported-By: Julien Grall <julien.grall@linaro.org>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Julien Grall <julien.grall@linaro.org>

Great, I'm able to compile libxc with gcc 4.8:

Tested-by: Julien Grall <julien.grall@linaro.org>

Regards,

-- 
Julien Grall

Re: [PATCH v2] libxc: Fix buffer length for get_suspend_file

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH v2] libxc: Fix buffer length for get_suspend_file"):
> On Tue, 2014-03-18 at 13:58 +0000, Ian Jackson wrote:
> > Declaring a formal parameter to have an array type doesn't result in
> > the parameter actually having an array type.  The type is "adjusted"
> > to a pointer.  (C99 6.9.1(7), 6.7.5.3.)
...
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Thanks, I've pushed it.

> Although I do wonder if it would be less error prone to push the size up
> to a parameter to get_suspend_file where it is (presumably) near to the
> declaration of the array.

I don't think so: this one-line function only has two call sites.  It
doesn't seem sensible to me to lift any of its contents into the call
sites in the name of clarity.

Ian.