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

[Ian Jackson <ian.jackson@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel