[Qemu-devel] [PATCH] migration: Replace strncpy() by g_strlcpy()

[Qemu-devel] [PATCH] migration: Replace strncpy() by g_strlcpy()

[Philippe Mathieu-Daudé]

Fedora 29 comes with GCC 8.1 which added the 'stringop-truncation' checks.

Replace the strncpy() calls by g_strlcpy() to avoid the following warning:

  migration/global_state.c: In function 'global_state_store_running':
  migration/global_state.c:45:5: error: 'strncpy' specified bound 100 equals destination size [-Werror=stringop-truncation]
       strncpy((char *)global_state.runstate,
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
              state, sizeof(global_state.runstate));
              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Reported-by: Howard Spoelstra <hsp.cat7@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
See http://lists.nongnu.org/archive/html/qemu-devel/2018-07/msg03723.html

 migration/global_state.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/global_state.c b/migration/global_state.c
index 8e8ab5c51e..d5df502cd5 100644
--- a/migration/global_state.c
+++ b/migration/global_state.c
@@ -42,7 +42,7 @@ int global_state_store(void)
 void global_state_store_running(void)
 {
     const char *state = RunState_str(RUN_STATE_RUNNING);
-    strncpy((char *)global_state.runstate,
+    g_strlcpy((char *)global_state.runstate,
            state, sizeof(global_state.runstate));
 }
 
-- 
2.18.0

Re: [Qemu-devel] [PATCH] migration: Replace strncpy() by g_strlcpy()

[Juan Quintela]

Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Fedora 29 comes with GCC 8.1 which added the 'stringop-truncation' checks.
>
> Replace the strncpy() calls by g_strlcpy() to avoid the following warning:
>
>   migration/global_state.c: In function 'global_state_store_running':
>   migration/global_state.c:45:5: error: 'strncpy' specified bound 100 equals destination size [-Werror=stringop-truncation]
>        strncpy((char *)global_state.runstate,
>        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>               state, sizeof(global_state.runstate));
>               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Reported-by: Howard Spoelstra <hsp.cat7@gmail.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Juan Quintela <quintela@redhat.com>

will be in next pull, thanks.

> ---
> See http://lists.nongnu.org/archive/html/qemu-devel/2018-07/msg03723.html
>
>  migration/global_state.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/migration/global_state.c b/migration/global_state.c
> index 8e8ab5c51e..d5df502cd5 100644
> --- a/migration/global_state.c
> +++ b/migration/global_state.c
> @@ -42,7 +42,7 @@ int global_state_store(void)
>  void global_state_store_running(void)
>  {
>      const char *state = RunState_str(RUN_STATE_RUNNING);
> -    strncpy((char *)global_state.runstate,
> +    g_strlcpy((char *)global_state.runstate,
>             state, sizeof(global_state.runstate));
>  }

Re: [Qemu-devel] [PATCH] migration: Replace strncpy() by g_strlcpy()

[David Hildenbrand]

On 18.08.2018 04:56, Philippe Mathieu-Daudé wrote:
> Fedora 29 comes with GCC 8.1 which added the 'stringop-truncation' checks.
> 
> Replace the strncpy() calls by g_strlcpy() to avoid the following warning:
> 
>   migration/global_state.c: In function 'global_state_store_running':
>   migration/global_state.c:45:5: error: 'strncpy' specified bound 100 equals destination size [-Werror=stringop-truncation]
>        strncpy((char *)global_state.runstate,
>        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>               state, sizeof(global_state.runstate));
>               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Reported-by: Howard Spoelstra <hsp.cat7@gmail.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> See http://lists.nongnu.org/archive/html/qemu-devel/2018-07/msg03723.html
> 
>  migration/global_state.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/migration/global_state.c b/migration/global_state.c
> index 8e8ab5c51e..d5df502cd5 100644
> --- a/migration/global_state.c
> +++ b/migration/global_state.c
> @@ -42,7 +42,7 @@ int global_state_store(void)
>  void global_state_store_running(void)
>  {
>      const char *state = RunState_str(RUN_STATE_RUNNING);
> -    strncpy((char *)global_state.runstate,
> +    g_strlcpy((char *)global_state.runstate,
>             state, sizeof(global_state.runstate));
>  }
>  
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH] migration: Replace strncpy() by g_strlcpy()

[Paolo Bonzini]

On 18/08/2018 04:56, Philippe Mathieu-Daudé wrote:
> Fedora 29 comes with GCC 8.1 which added the 'stringop-truncation' checks.
> 
> Replace the strncpy() calls by g_strlcpy() to avoid the following warning:
> 
>   migration/global_state.c: In function 'global_state_store_running':
>   migration/global_state.c:45:5: error: 'strncpy' specified bound 100 equals destination size [-Werror=stringop-truncation]
>        strncpy((char *)global_state.runstate,
>        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>               state, sizeof(global_state.runstate));
>               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Reported-by: Howard Spoelstra <hsp.cat7@gmail.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> See http://lists.nongnu.org/archive/html/qemu-devel/2018-07/msg03723.html
> 
>  migration/global_state.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/migration/global_state.c b/migration/global_state.c
> index 8e8ab5c51e..d5df502cd5 100644
> --- a/migration/global_state.c
> +++ b/migration/global_state.c
> @@ -42,7 +42,7 @@ int global_state_store(void)
>  void global_state_store_running(void)
>  {
>      const char *state = RunState_str(RUN_STATE_RUNNING);
> -    strncpy((char *)global_state.runstate,
> +    g_strlcpy((char *)global_state.runstate,
>             state, sizeof(global_state.runstate));
>  }
>  
> 

This is wrong because strlcpy doesn't zero the rest of
global_state.runstate, so you could end up with something like
"running\0ate\0\0..." in global_state.runstate However, the same mistake
is already present in vl.c's runstate_store.

Juan, David, what to do?  strncpy is easy to misuse, but we do have
cases where it's correct and it should tingle the reviewers' spidey
senses...  I wouldn't mind disabling the warning, and using strncpy in
runstate_store, because in practice it's already reported by Coverity
and it can be shut up there.

Thanks,

Paolo

Re: [Qemu-devel] [PATCH] migration: Replace strncpy() by g_strlcpy()

[David Hildenbrand]

On 20.08.2018 15:23, Paolo Bonzini wrote:
> On 18/08/2018 04:56, Philippe Mathieu-Daudé wrote:
>> Fedora 29 comes with GCC 8.1 which added the 'stringop-truncation' checks.
>>
>> Replace the strncpy() calls by g_strlcpy() to avoid the following warning:
>>
>>   migration/global_state.c: In function 'global_state_store_running':
>>   migration/global_state.c:45:5: error: 'strncpy' specified bound 100 equals destination size [-Werror=stringop-truncation]
>>        strncpy((char *)global_state.runstate,
>>        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>               state, sizeof(global_state.runstate));
>>               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> Reported-by: Howard Spoelstra <hsp.cat7@gmail.com>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> See http://lists.nongnu.org/archive/html/qemu-devel/2018-07/msg03723.html
>>
>>  migration/global_state.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/migration/global_state.c b/migration/global_state.c
>> index 8e8ab5c51e..d5df502cd5 100644
>> --- a/migration/global_state.c
>> +++ b/migration/global_state.c
>> @@ -42,7 +42,7 @@ int global_state_store(void)
>>  void global_state_store_running(void)
>>  {
>>      const char *state = RunState_str(RUN_STATE_RUNNING);
>> -    strncpy((char *)global_state.runstate,
>> +    g_strlcpy((char *)global_state.runstate,
>>             state, sizeof(global_state.runstate));
>>  }
>>  
>>
> 
> This is wrong because strlcpy doesn't zero the rest of

Two RB-s and it is still wrong implies that string operations are still
the root of all evil. :)

> global_state.runstate, so you could end up with something like
> "running\0ate\0\0..." in global_state.runstate However, the same mistake
> is already present in vl.c's runstate_store.
> 
> Juan, David, what to do?  strncpy is easy to misuse, but we do have
> cases where it's correct and it should tingle the reviewers' spidey
> senses...  I wouldn't mind disabling the warning, and using strncpy in
> runstate_store, because in practice it's already reported by Coverity
> and it can be shut up there.

Maybe really set it to zero (memset) before using the g_strlcpy? I am
not a fan of disabling warnings, but if you think this is
easier/cleaner, let's go for that.

> 
> Thanks,
> 
> Paolo
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH] migration: Replace strncpy() by g_strlcpy()

[Thomas Huth]

On 2018-08-20 15:28, David Hildenbrand wrote:
> On 20.08.2018 15:23, Paolo Bonzini wrote:
>> On 18/08/2018 04:56, Philippe Mathieu-Daudé wrote:
>>> Fedora 29 comes with GCC 8.1 which added the 'stringop-truncation' checks.
>>>
>>> Replace the strncpy() calls by g_strlcpy() to avoid the following warning:
>>>
>>>   migration/global_state.c: In function 'global_state_store_running':
>>>   migration/global_state.c:45:5: error: 'strncpy' specified bound 100 equals destination size [-Werror=stringop-truncation]
>>>        strncpy((char *)global_state.runstate,
>>>        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>               state, sizeof(global_state.runstate));
>>>               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>> Reported-by: Howard Spoelstra <hsp.cat7@gmail.com>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>> See http://lists.nongnu.org/archive/html/qemu-devel/2018-07/msg03723.html
>>>
>>>  migration/global_state.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/migration/global_state.c b/migration/global_state.c
>>> index 8e8ab5c51e..d5df502cd5 100644
>>> --- a/migration/global_state.c
>>> +++ b/migration/global_state.c
>>> @@ -42,7 +42,7 @@ int global_state_store(void)
>>>  void global_state_store_running(void)
>>>  {
>>>      const char *state = RunState_str(RUN_STATE_RUNNING);
>>> -    strncpy((char *)global_state.runstate,
>>> +    g_strlcpy((char *)global_state.runstate,
>>>             state, sizeof(global_state.runstate));
>>>  }
>>>  
>>>
>>
>> This is wrong because strlcpy doesn't zero the rest of
> 
> Two RB-s and it is still wrong implies that string operations are still
> the root of all evil. :)
> 
>> global_state.runstate, so you could end up with something like
>> "running\0ate\0\0..." in global_state.runstate However, the same mistake
>> is already present in vl.c's runstate_store.
>>
>> Juan, David, what to do?  strncpy is easy to misuse, but we do have
>> cases where it's correct and it should tingle the reviewers' spidey
>> senses...  I wouldn't mind disabling the warning, and using strncpy in
>> runstate_store, because in practice it's already reported by Coverity
>> and it can be shut up there.
> 
> Maybe really set it to zero (memset) before using the g_strlcpy? I am
> not a fan of disabling warnings, but if you think this is
> easier/cleaner, let's go for that.

FWIW, that new warning from GCC is IMHO just annoying. I had the same
problem in the SLOF sources, too:

https://github.com/aik/SLOF/commit/d8a9354c2a35136

The code with strncpy was perfectly valid before, but to get rid of the
warning, I replaced it with a more cumbersome memcpy instead (and mad
sure that the memory is already cleared earlier in that function). Now
seeing that the problem with strncpy pops up here, too, I think it would
maybe be better to shut up the warning of GCC, since it's clearly GCC
who's wrong here.

 Thomas

Re: [Qemu-devel] [PATCH] migration: Replace strncpy() by g_strlcpy()

[Eric Blake]

On 08/20/2018 12:16 PM, Thomas Huth wrote:

>>
>> Maybe really set it to zero (memset) before using the g_strlcpy? I am
>> not a fan of disabling warnings, but if you think this is
>> easier/cleaner, let's go for that.

I'm not a fan of strlcpy in general (by the time you've properly set it 
up to detect/report/avoid truncation errors, you've added more 
boilerplate code than you would have by just doing memcpy() yourself).

> 
> FWIW, that new warning from GCC is IMHO just annoying. I had the same
> problem in the SLOF sources, too:
> 
> https://github.com/aik/SLOF/commit/d8a9354c2a35136
> 
> The code with strncpy was perfectly valid before, but to get rid of the
> warning, I replaced it with a more cumbersome memcpy instead (and mad
> sure that the memory is already cleared earlier in that function). Now
> seeing that the problem with strncpy pops up here, too, I think it would
> maybe be better to shut up the warning of GCC, since it's clearly GCC
> who's wrong here.

gcc is not necessarily wrong, as it CAN catch real erroneous uses of 
strncpy().  It's just that 99% of the time, strncpy() is the WRONG 
function to use, and so the remaining few cases where it actually does 
what you want are so rare that you have to consult the manual anyways. 
If nothing else, the gcc warning is making people avoid strncpy() even 
where it is safe (which is not a bad thing, in my opinion, because the 
contract of strncpy() is so counter-intuitive).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH] migration: Replace strncpy() by g_strlcpy()

[David Hildenbrand]

On 20.08.2018 21:48, Eric Blake wrote:
> On 08/20/2018 12:16 PM, Thomas Huth wrote:
> 
>>>
>>> Maybe really set it to zero (memset) before using the g_strlcpy? I am
>>> not a fan of disabling warnings, but if you think this is
>>> easier/cleaner, let's go for that.
> 
> I'm not a fan of strlcpy in general (by the time you've properly set it 
> up to detect/report/avoid truncation errors, you've added more 
> boilerplate code than you would have by just doing memcpy() yourself).
> 
>>
>> FWIW, that new warning from GCC is IMHO just annoying. I had the same
>> problem in the SLOF sources, too:
>>
>> https://github.com/aik/SLOF/commit/d8a9354c2a35136
>>
>> The code with strncpy was perfectly valid before, but to get rid of the
>> warning, I replaced it with a more cumbersome memcpy instead (and mad
>> sure that the memory is already cleared earlier in that function). Now
>> seeing that the problem with strncpy pops up here, too, I think it would
>> maybe be better to shut up the warning of GCC, since it's clearly GCC
>> who's wrong here.
> 
> gcc is not necessarily wrong, as it CAN catch real erroneous uses of 
> strncpy().  It's just that 99% of the time, strncpy() is the WRONG 
> function to use, and so the remaining few cases where it actually does 
> what you want are so rare that you have to consult the manual anyways. 
> If nothing else, the gcc warning is making people avoid strncpy() even 
> where it is safe (which is not a bad thing, in my opinion, because the 
> contract of strncpy() is so counter-intuitive).
> 

I am wondering if we should simply add a helper for these special cases
that zeroes the buffer and uses g_strlcpy(), instead of
ignoring/disabling the warning.

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH] migration: Replace strncpy() by g_strlcpy()

[Thomas Huth]

On 2018-08-20 21:59, David Hildenbrand wrote:
> On 20.08.2018 21:48, Eric Blake wrote:
>> On 08/20/2018 12:16 PM, Thomas Huth wrote:
>>
>>>>
>>>> Maybe really set it to zero (memset) before using the g_strlcpy? I am
>>>> not a fan of disabling warnings, but if you think this is
>>>> easier/cleaner, let's go for that.
>>
>> I'm not a fan of strlcpy in general (by the time you've properly set it 
>> up to detect/report/avoid truncation errors, you've added more 
>> boilerplate code than you would have by just doing memcpy() yourself).
>>
>>>
>>> FWIW, that new warning from GCC is IMHO just annoying. I had the same
>>> problem in the SLOF sources, too:
>>>
>>> https://github.com/aik/SLOF/commit/d8a9354c2a35136
>>>
>>> The code with strncpy was perfectly valid before, but to get rid of the
>>> warning, I replaced it with a more cumbersome memcpy instead (and mad
>>> sure that the memory is already cleared earlier in that function). Now
>>> seeing that the problem with strncpy pops up here, too, I think it would
>>> maybe be better to shut up the warning of GCC, since it's clearly GCC
>>> who's wrong here.
>>
>> gcc is not necessarily wrong, as it CAN catch real erroneous uses of 
>> strncpy().  It's just that 99% of the time, strncpy() is the WRONG 
>> function to use, and so the remaining few cases where it actually does 
>> what you want are so rare that you have to consult the manual anyways. 
>> If nothing else, the gcc warning is making people avoid strncpy() even 
>> where it is safe (which is not a bad thing, in my opinion, because the 
>> contract of strncpy() is so counter-intuitive).
>>
> 
> I am wondering if we should simply add a helper for these special cases
> that zeroes the buffer and uses g_strlcpy(), instead of
> ignoring/disabling the warning.

Yes, a helper function with a proper comment about its purpose is likely
the best way to go.

 Thomas

Re: [Qemu-devel] [PATCH] migration: Replace strncpy() by g_strlcpy()

[Paolo Bonzini]

On 21/08/2018 08:03, Thomas Huth wrote:
>>> gcc is not necessarily wrong, as it CAN catch real erroneous uses of 
>>> strncpy().  It's just that 99% of the time, strncpy() is the WRONG 
>>> function to use, and so the remaining few cases where it actually does 
>>> what you want are so rare that you have to consult the manual anyways. 
>>> If nothing else, the gcc warning is making people avoid strncpy() even 
>>> where it is safe (which is not a bad thing, in my opinion, because the 
>>> contract of strncpy() is so counter-intuitive).
>>>
>> I am wondering if we should simply add a helper for these special cases
>> that zeroes the buffer and uses g_strlcpy(), instead of
>> ignoring/disabling the warning.
> Yes, a helper function with a proper comment about its purpose is likely
> the best way to go.

But why use g_strlcpy in the function (which has an off-by-one effect)?

Maybe it could be a qemu_strncpy that is the same as strncpy but returns
-ERANGE if the source is longer than the buffer that is passed in (but
not if it fits perfectly without a terminating NUL):

    int qemu_strncpy(const char *d, const char *s, int dsize)
    {
        while (*s && dsize) {
            *d++ = *s++;
            dsize--;
        }
        /* It's okay if D is just past the end of the buffer,
         * and S is sitting on the terminating NUL.
         */
        if (*s) {
            return -ERANGE;
        }
        while (dsize) {
            *d++ = 0;
        }
        return 0;
    }

Then we have the problem of yet another incomplete transition, but at
least we could convert those that GCC complains about.

Paolo

Re: [Qemu-devel] [PATCH] migration: Replace strncpy() by g_strlcpy()

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 21/08/2018 08:03, Thomas Huth wrote:
>>>> gcc is not necessarily wrong, as it CAN catch real erroneous uses of 
>>>> strncpy().  It's just that 99% of the time, strncpy() is the WRONG 
>>>> function to use, and so the remaining few cases where it actually does 
>>>> what you want are so rare that you have to consult the manual anyways. 
>>>> If nothing else, the gcc warning is making people avoid strncpy() even 
>>>> where it is safe (which is not a bad thing, in my opinion, because the 
>>>> contract of strncpy() is so counter-intuitive).
>>>>
>>> I am wondering if we should simply add a helper for these special cases
>>> that zeroes the buffer and uses g_strlcpy(), instead of
>>> ignoring/disabling the warning.
>> Yes, a helper function with a proper comment about its purpose is likely
>> the best way to go.
>
> But why use g_strlcpy in the function (which has an off-by-one effect)?
>
> Maybe it could be a qemu_strncpy that is the same as strncpy but returns
> -ERANGE if the source is longer than the buffer that is passed in (but
> not if it fits perfectly without a terminating NUL):
>
>     int qemu_strncpy(const char *d, const char *s, int dsize)
>     {
>         while (*s && dsize) {
>             *d++ = *s++;
>             dsize--;
>         }
>         /* It's okay if D is just past the end of the buffer,
>          * and S is sitting on the terminating NUL.
>          */
>         if (*s) {
>             return -ERANGE;
>         }
>         while (dsize) {
>             *d++ = 0;
              dsize--;  ?????
>         }
>         return 0;
>     }
>
> Then we have the problem of yet another incomplete transition, but at
> least we could convert those that GCC complains about.

I think this is the best approach.  At least we have a semantic that is
"clear", as trivial as:
- we copy a maximum of dsize chars
- source size has to be at maximum dsize
- we pad destination if size of source is smaller than dsize
- destination could be not zero terminated if source is exactly dsize
  length.

And people wonders why I consider C unsuited to handle strings.

Later, Juan.

PD.  I think that adding a strncpy that don't behave like strncpy is a
     bad idea.  What about strlncpy()?  I.e. does the work of both?  As
     I have no clue what the "l" on strlcpy stands for, we can choose
     any other letter.