[PATCH] tools: fix several "format-truncation" errors with GCC 7

[PATCH] tools: fix several "format-truncation" errors with GCC 7

[Zhongze Liu]

replace several snprintf with asprintf in xenpmd and tools/ocmal/xc
to fix the "format-truncation" errors caused by incorrect size of buffers.

Signed-off-by: Zhongze Liu <blackskygg@gmail.com>
---
CC: David Scott <dave@recoil.org>
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/ocaml/libs/xc/xenctrl_stubs.c | 12 +++++-------
 tools/xenpmd/xenpmd.c               | 18 ++++++++++++------
 2 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index 5e455519d4..7011930360 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -14,6 +14,7 @@
  * GNU Lesser General Public License for more details.
  */
 
+#define _GNU_SOURCE
 #define _XOPEN_SOURCE 600
 #include <stdlib.h>
 #include <errno.h>
@@ -54,20 +55,17 @@
 
 static void Noreturn failwith_xc(xc_interface *xch)
 {
-	char error_str[256];
+	char *error_str = NULL;
 	if (xch) {
 		const xc_error *error = xc_get_last_error(xch);
 		if (error->code == XC_ERROR_NONE)
-			snprintf(error_str, sizeof(error_str),
-				 "%d: %s", errno, strerror(errno));
+			asprintf(&error_str, "%d: %s", errno, strerror(errno));
 		else
-			snprintf(error_str, sizeof(error_str),
-				 "%d: %s: %s", error->code,
+			asprintf(&error_str, "%d: %s: %s", error->code,
 				 xc_error_code_to_desc(error->code),
 				 error->message);
 	} else {
-		snprintf(error_str, sizeof(error_str),
-			 "Unable to open XC interface");
+		asprintf(&error_str, "Unable to open XC interface");
 	}
 	caml_raise_with_string(*caml_named_value("xc.error"), error_str);
 }
diff --git a/tools/xenpmd/xenpmd.c b/tools/xenpmd/xenpmd.c
index b3a31062aa..2f4ef45a11 100644
--- a/tools/xenpmd/xenpmd.c
+++ b/tools/xenpmd/xenpmd.c
@@ -32,6 +32,7 @@
  * passed to the guest when appropriate battery ports are read/written to.
  */
 
+#define _GNU_SOURCE
 #include <stdio.h>
 #include <stdarg.h>
 #include <string.h>
@@ -100,7 +101,8 @@ FILE *get_next_battery_file(DIR *battery_dir,
 {
     FILE *file = 0;
     struct dirent *dir_entries;
-    char file_name[32];
+    char *file_name = NULL;
+    int rc = 0;
     
     do 
     {
@@ -110,12 +112,16 @@ FILE *get_next_battery_file(DIR *battery_dir,
         if ( strlen(dir_entries->d_name) < 4 )
             continue;
         if ( battery_info_type == BIF ) 
-            snprintf(file_name, 32, BATTERY_INFO_FILE_PATH,
-                     dir_entries->d_name);
+	    rc = asprintf(&file_name, BATTERY_INFO_FILE_PATH,
+			  dir_entries->d_name);
         else 
-            snprintf(file_name, 32, BATTERY_STATE_FILE_PATH,
-                     dir_entries->d_name);
-        file = fopen(file_name, "r");
+            rc = asprintf(&file_name, BATTERY_STATE_FILE_PATH,
+			  dir_entries->d_name);
+
+	if ( -1 != rc ) {
+	    file = fopen(file_name, "r");
+	    free(file_name);
+	}
     } while ( !file );
 
     return file;
-- 
2.13.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] tools: fix several "format-truncation" errors with GCC 7

[Ian Jackson]

Zhongze Liu writes ("[PATCH] tools: fix several "format-truncation" errors with GCC 7"):
> replace several snprintf with asprintf in xenpmd and tools/ocmal/xc
> to fix the "format-truncation" errors caused by incorrect size of buffers.

Thanks for paying attention to the quality of our code, but:

I wonder whether this cure is worse than the disease.  Using asprintf
everywhere means additional error handling (which you have erroneously
omitted) and additional potential for leaks etc. (for which I haven't
analysed your patch).

You say `"format-truncation" errors' but you mean compiler warnings
from -Wformat-truncation, turned into errors by -Werror.  Is there
any suggestion from a human that this code actually malfunctions ?

Or does the compiler not just complain all the time about snprintf ?

> -	char error_str[256];
...
> -			snprintf(error_str, sizeof(error_str),
> -				 "%d: %s", errno, strerror(errno));

This will not truncate unless the xc error string is too long, which
is not.

> -		snprintf(error_str, sizeof(error_str),
> -			 "Unable to open XC interface");
> +		asprintf(&error_str, "Unable to open XC interface");

This is a fixed string of course.

> -    char file_name[32];
...
> @@ -110,12 +112,16 @@ FILE *get_next_battery_file(DIR *battery_dir,
>          if ( strlen(dir_entries->d_name) < 4 )
>              continue;
>          if ( battery_info_type == BIF ) 
> -            snprintf(file_name, 32, BATTERY_INFO_FILE_PATH,
> -                     dir_entries->d_name);
> +	    rc = asprintf(&file_name, BATTERY_INFO_FILE_PATH,
> +			  dir_entries->d_name);
>          else 
> -            snprintf(file_name, 32, BATTERY_STATE_FILE_PATH,
> -                     dir_entries->d_name);
> -        file = fopen(file_name, "r");
> +            rc = asprintf(&file_name, BATTERY_STATE_FILE_PATH,
> +			  dir_entries->d_name);

These filenames are all very formulaic.  I doubt they are being
truncated even though the limit is only 32 bytes.

Regards,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] tools: fix several "format-truncation" errors with GCC 7

[Zhongze Liu]

Hi Ian,

Thanks for pointing out the problems. I've consulted several
maintainers about this
and have drafted a new patch for it (in a new [patch v2] thread).
Please have a look
at it. Thanks.

Cheers,

Zhongze Liu.

2017-06-12 20:31 GMT+08:00 Ian Jackson <ian.jackson@eu.citrix.com>:
> Zhongze Liu writes ("[PATCH] tools: fix several "format-truncation" errors with GCC 7"):
>> replace several snprintf with asprintf in xenpmd and tools/ocmal/xc
>> to fix the "format-truncation" errors caused by incorrect size of buffers.
>
> Thanks for paying attention to the quality of our code, but:
>
> I wonder whether this cure is worse than the disease.  Using asprintf
> everywhere means additional error handling (which you have erroneously
> omitted) and additional potential for leaks etc. (for which I haven't
> analysed your patch).
>
> You say `"format-truncation" errors' but you mean compiler warnings
> from -Wformat-truncation, turned into errors by -Werror.  Is there
> any suggestion from a human that this code actually malfunctions ?
>
> Or does the compiler not just complain all the time about snprintf ?
>
>> -     char error_str[256];
> ...
>> -                     snprintf(error_str, sizeof(error_str),
>> -                              "%d: %s", errno, strerror(errno));
>
> This will not truncate unless the xc error string is too long, which
> is not.
>
>> -             snprintf(error_str, sizeof(error_str),
>> -                      "Unable to open XC interface");
>> +             asprintf(&error_str, "Unable to open XC interface");
>
> This is a fixed string of course.
>
>> -    char file_name[32];
> ...
>> @@ -110,12 +112,16 @@ FILE *get_next_battery_file(DIR *battery_dir,
>>          if ( strlen(dir_entries->d_name) < 4 )
>>              continue;
>>          if ( battery_info_type == BIF )
>> -            snprintf(file_name, 32, BATTERY_INFO_FILE_PATH,
>> -                     dir_entries->d_name);
>> +         rc = asprintf(&file_name, BATTERY_INFO_FILE_PATH,
>> +                       dir_entries->d_name);
>>          else
>> -            snprintf(file_name, 32, BATTERY_STATE_FILE_PATH,
>> -                     dir_entries->d_name);
>> -        file = fopen(file_name, "r");
>> +            rc = asprintf(&file_name, BATTERY_STATE_FILE_PATH,
>> +                       dir_entries->d_name);
>
> These filenames are all very formulaic.  I doubt they are being
> truncated even though the limit is only 32 bytes.
>
> Regards,
> Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel