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

[Zhongze Liu <blackskygg@xxxxxxxxx>]

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@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