On 11.01.21 14:17, Miroslav Rezanina wrote:
>
>
> ----- Original Message -----
>> From: "Christian Borntraeger" <[email protected]>
>> To: "Thomas Huth" <[email protected]>, "Miroslav Rezanina"
>> <[email protected]>
>> Cc: "qemu-s390x" <[email protected]>, "Philippe Mathieu-Daudé"
>> <[email protected]>, [email protected]
>> Sent: Monday, January 11, 2021 2:02:32 PM
>> Subject: Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length
>>
>>
>>
>> On 11.01.21 13:54, Thomas Huth wrote:
>>> On 11/01/2021 13.42, Miroslav Rezanina wrote:
>>>>
>>>>
>>>> ----- Original Message -----
>>>>> From: "Thomas Huth" <[email protected]>
>>>>> To: "Philippe Mathieu-Daudé" <[email protected]>, [email protected],
>>>>> [email protected], "qemu-s390x"
>>>>> <[email protected]>
>>>>> Sent: Monday, January 11, 2021 1:24:57 PM
>>>>> Subject: Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length
>>>>>
>>>>> On 11/01/2021 13.10, Philippe Mathieu-Daudé wrote:
>>>>>> Hi Miroslav,
>>>>>>
>>>>>> On 1/11/21 12:30 PM, [email protected] wrote:
>>>>>>> From: Miroslav Rezanina <[email protected]>
>>>>>>>
>>>>>>> There are two cases when vm name is copied but closing \0 can be lost
>>>>>>> in case name is too long (>=256 characters).
>>>>>>>
>>>>>>> Updating length to copy so there is space for closing \0.
>>>>>>>
>>>>>>> Signed-off-by: Miroslav Rezanina <[email protected]>
>>>>>>> ---
>>>>>>> target/s390x/kvm.c | 2 +-
>>>>>>> target/s390x/misc_helper.c | 4 +++-
>>>>>>> 2 files changed, 4 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>>>>>>> index b8385e6b95..2313b5727e 100644
>>>>>>> --- a/target/s390x/kvm.c
>>>>>>> +++ b/target/s390x/kvm.c
>>>>>>> @@ -1918,7 +1918,7 @@ static void insert_stsi_3_2_2(S390CPU *cpu, __u64
>>>>>>> addr, uint8_t ar)
>>>>>>> */
>>>>>>> if (qemu_name) {
>>>>>>> strncpy((char *)sysib.ext_names[0], qemu_name,
>>>>>>> - sizeof(sysib.ext_names[0]));
>>>>>>> + sizeof(sysib.ext_names[0]) - 1);
>>>>>>> } else {
>>>>>>> strcpy((char *)sysib.ext_names[0], "KVMguest");
>>>>>>> }
>>>>>>
>>>>>> What about using strpadcpy() instead?
>>>>>
>>>>> Yes, strpadcpy is the better way here - this field has to be padded with
>>>>> zeroes, so doing "- 1" is wrong here.
>>>>
>>>> Hi Thomas,
>>>>
>>>> as I wrote in reply to Phillipe - the array is memset to zeroes before the
>>>> if so we
>>>> are sure it's padded with zeroes (in this occurrence, not true for second
>>>> one).
>>>
>>> Ok, but dropping the last character is still wrong here. The ext_names do
>>> not need to be terminated with a \0 if they have the full length.
>> The current code is actually correct. We are perfectly fine without the final
>> \n if the string is really 256 bytes.
>>
>> Replacing memset + strncpy with strpadcpy is certainly a good cleanup. Is it
>> necessary? No.
>
> Yes, it is necessary because otherwise compiler (GCC 11) produce warning and
> so
> build fail when --enable-werror is used.
Fair enough. But that actually means that the compiler warning is wrong, because
we use strncpy exactly in the way as described (allowing for the final \n to be
missing). But let us use strpadcpy then.
