Re: [Xen-devel] [PATCH] hvmloader/smbios: Change strncpy to memcpy for anchor strings.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.09.13 at 11:14, Paul Durrant <Paul.Durrant@xxxxxxxxxx> wrote:
>>  -----Original Message-----
>> From: xen-devel-bounces@xxxxxxxxxxxxx [mailto:xen-devel-
>> bounces@xxxxxxxxxxxxx] On Behalf Of Andrew Cooper
>> Sent: 23 September 2013 19:22
>> To: Xen-devel
>> Cc: Andrew Cooper; Keir (Xen.org); Jan Beulich
>> Subject: [Xen-devel] [PATCH] hvmloader/smbios: Change strncpy to
>> memcpy for anchor strings.
>> 
>> Coverity complains about the use of strncpy() to completely fill the anchor
>> strings, resulting in an unterminated string.
>> 
>> Although the strncpy result is correct, the anchor strings are not strings 
> in
>> the C sense, and use of memcpy is the prevaling style elsewhere in
>> hvmloader
>> anyway.
>> 
>> While tidying up the style in this function, also remove some trailing
>> whitespace and gratuitous cast.
>> 
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> CC: Keir Fraser <keir@xxxxxxx>
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> ---
>>  tools/firmware/hvmloader/smbios.c |    9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>> 
>> diff --git a/tools/firmware/hvmloader/smbios.c
>> b/tools/firmware/hvmloader/smbios.c
>> index 9f292cc..900f4e7 100644
>> --- a/tools/firmware/hvmloader/smbios.c
>> +++ b/tools/firmware/hvmloader/smbios.c
>> @@ -347,18 +347,19 @@ smbios_entry_point_init(void *start,
>>  {
>>      uint8_t sum;
>>      int i;
>> -    struct smbios_entry_point *ep = (struct smbios_entry_point *)start;
>> +    struct smbios_entry_point *ep = start;
>> 
>>      memset(ep, 0, sizeof(*ep));
>> 
>> -    strncpy(ep->anchor_string, "_SM_", 4);
>> +    memcpy(ep->anchor_string, "_SM_", sizeof(ep->anchor_string));
> 
> Why the change from 4 to sizeof(ep->anchor_string) here (and similar below)? 
> Setting the copy length based on the size of the destination rather than the 
> source seems like the wrong thing to do.

One can argue either way here:
- passing the destination's size guarantees no memory corruption
- passing the source's size guarantees no uninitialized memory

Since the structure fields involved here aren't going to change,
either way is fine imo.

Jan


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