(note this is how other functions also handle the errors).

hw/9pfs/9p.c:948:18: warning: Loss of sign in implicit conversion
        offset = err;
                 ^~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/9pfs/9p.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 333dbb6f8e..0a37c8bd13 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -945,7 +945,6 @@ static void coroutine_fn v9fs_version(void *opaque)
     v9fs_string_init(&version);
     err = pdu_unmarshal(pdu, offset, "ds", &s->msize, &version);
     if (err < 0) {
-        offset = err;
         goto out;
     }
     trace_v9fs_version(pdu->tag, pdu->id, s->msize, version.data);
@@ -962,13 +961,12 @@ static void coroutine_fn v9fs_version(void *opaque)
 
     err = pdu_marshal(pdu, offset, "ds", s->msize, &version);
     if (err < 0) {
-        offset = err;
         goto out;
     }
-    offset += err;
+    err += offset;
     trace_v9fs_version_return(pdu->tag, pdu->id, s->msize, version.data);
 out:
-    pdu_complete(pdu, offset);
+    pdu_complete(pdu, err);
     v9fs_string_free(&version);
 }
 
-- 
2.13.3

On Wed, 26 Jul 2017 23:42:22 -0300
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> (note this is how other functions also handle the errors).
> 
> hw/9pfs/9p.c:948:18: warning: Loss of sign in implicit conversion
>         offset = err;
>                  ^~~
> 
> Reported-by: Clang Static Analyzer
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

Now, I'm not sure this can be merged during hard freeze since it is
more code cleanup than actual bug fixing...

>  hw/9pfs/9p.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 333dbb6f8e..0a37c8bd13 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -945,7 +945,6 @@ static void coroutine_fn v9fs_version(void *opaque)
>      v9fs_string_init(&version);
>      err = pdu_unmarshal(pdu, offset, "ds", &s->msize, &version);
>      if (err < 0) {
> -        offset = err;
>          goto out;
>      }
>      trace_v9fs_version(pdu->tag, pdu->id, s->msize, version.data);
> @@ -962,13 +961,12 @@ static void coroutine_fn v9fs_version(void *opaque)
>  
>      err = pdu_marshal(pdu, offset, "ds", s->msize, &version);
>      if (err < 0) {
> -        offset = err;
>          goto out;
>      }
> -    offset += err;
> +    err += offset;
>      trace_v9fs_version_return(pdu->tag, pdu->id, s->msize, version.data);
>  out:
> -    pdu_complete(pdu, offset);
> +    pdu_complete(pdu, err);
>      v9fs_string_free(&version);
>  }
>  

On 07/27/2017 08:40 AM, Greg Kurz wrote:
> On Wed, 26 Jul 2017 23:42:22 -0300
> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> Now, I'm not sure this can be merged during hard freeze since it is
> more code cleanup than actual bug fixing...

Hmm the commit message is probably not enough.
The problem is this code can send broken packets, see inlined:

> 
>>   hw/9pfs/9p.c | 6 ++----
>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
>> index 333dbb6f8e..0a37c8bd13 100644
>> --- a/hw/9pfs/9p.c
>> +++ b/hw/9pfs/9p.c
>> @@ -945,7 +945,6 @@ static void coroutine_fn v9fs_version(void *opaque)
>>       v9fs_string_init(&version);
>>       err = pdu_unmarshal(pdu, offset, "ds", &s->msize, &version);
>>       if (err < 0) {

if err == -1

>> -        offset = err;

here this sets offset = (size_t)(-1) = SIZE_MAX

>>           goto out;
>>       }
>>       trace_v9fs_version(pdu->tag, pdu->id, s->msize, version.data);
>> @@ -962,13 +961,12 @@ static void coroutine_fn v9fs_version(void *opaque)
>>   
>>       err = pdu_marshal(pdu, offset, "ds", s->msize, &version);
>>       if (err < 0) {
>> -        offset = err;
>>           goto out;
>>       }
>> -    offset += err;

here offset += SIZE_MAX which wraps, so this equivs to offset -= 1

>> +    err += offset;
>>       trace_v9fs_version_return(pdu->tag, pdu->id, s->msize, version.data);
>>   out:
>> -    pdu_complete(pdu, offset);

now we have offset = 7 - 1 = 6, since 6 > 0 pdu_complete() does not 
marshal the error code but 6 bytes of crap from pdu?

Maybe I missed something.

>> +    pdu_complete(pdu, err);
>>       v9fs_string_free(&version);
>>   }
>>   

Regards,

Phil.

On Thu, 27 Jul 2017 16:18:07 -0300
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> On 07/27/2017 08:40 AM, Greg Kurz wrote:
> > On Wed, 26 Jul 2017 23:42:22 -0300
> > Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> > 
> > Now, I'm not sure this can be merged during hard freeze since it is
> > more code cleanup than actual bug fixing...  
> 
> Hmm the commit message is probably not enough.
> The problem is this code can send broken packets, see inlined:
> 
> >   
> >>   hw/9pfs/9p.c | 6 ++----
> >>   1 file changed, 2 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> >> index 333dbb6f8e..0a37c8bd13 100644
> >> --- a/hw/9pfs/9p.c
> >> +++ b/hw/9pfs/9p.c
> >> @@ -945,7 +945,6 @@ static void coroutine_fn v9fs_version(void *opaque)
> >>       v9fs_string_init(&version);
> >>       err = pdu_unmarshal(pdu, offset, "ds", &s->msize, &version);
> >>       if (err < 0) {  
> 
> if err == -1
> 
> >> -        offset = err;  
> 
> here this sets offset = (size_t)(-1) = SIZE_MAX
> 
> >>           goto out;

and here we jump directly to the out label, so...

> >>       }
> >>       trace_v9fs_version(pdu->tag, pdu->id, s->msize, version.data);
> >> @@ -962,13 +961,12 @@ static void coroutine_fn v9fs_version(void *opaque)
> >>   
> >>       err = pdu_marshal(pdu, offset, "ds", s->msize, &version);
> >>       if (err < 0) {
> >> -        offset = err;
> >>           goto out;
> >>       }
> >> -    offset += err;  
> 
> here offset += SIZE_MAX which wraps, so this equivs to offset -= 1
> 

... this cannot happen.

> >> +    err += offset;
> >>       trace_v9fs_version_return(pdu->tag, pdu->id, s->msize, version.data);
> >>   out:
> >> -    pdu_complete(pdu, offset);  
> 
> now we have offset = 7 - 1 = 6, since 6 > 0 pdu_complete() does not 
> marshal the error code but 6 bytes of crap from pdu?
> 
> Maybe I missed something.
> 
> >> +    pdu_complete(pdu, err);
> >>       v9fs_string_free(&version);
> >>   }
> >>     
> 
> Regards,
> 
> Phil.

I've pushed this to my 9p-next branch to be merged in 2.11.

Cheers,

--
Greg