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

2017-06-12 Thread 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 :
> 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(_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(_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(_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: [Xen-devel] [PATCH] tools: fix several "format-truncation" errors with GCC 7

2017-06-12 Thread 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(_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(_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(_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

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

2017-06-12 Thread 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 
---
CC: David Scott 
CC: Ian Jackson 
CC: Wei Liu 
---
 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 
 #include 
@@ -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(_str, "%d: %s", errno, strerror(errno));
else
-   snprintf(error_str, sizeof(error_str),
-"%d: %s: %s", error->code,
+   asprintf(_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(_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 
 #include 
 #include 
@@ -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(_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(_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