Re: [Qemu-devel] [patch] linux-user/syscall.c: Fix missing break for host_to_target_cmsg

[Nageswara R Sastry <rnsastry@linux.vnet.ibm.com>]

On 2018-02-15 20:17, Laurent Vivier wrote:
> Le 08/02/2018 à 10:56, Nageswara R Sastry a écrit :
>> On 2018-02-07 19:27, Laurent Vivier wrote:
>>> Le 07/02/2018 à 10:49, no-reply@patchew.org a écrit :
>>>> Hi,
>>>> 
>>>> This series failed build test on s390x host. Please find the details
>>>> below.
>>> ...
>>>>   CC      aarch64_be-linux-user/linux-user/syscall.o
>>>> In file included from
>>>> /var/tmp/patchew-tester-tmp-ewjgn083/src/linux-user/qemu.h:16:0,
>>>>                  from
>>>> /var/tmp/patchew-tester-tmp-ewjgn083/src/linux-user/syscall.c:118:
>>>> /var/tmp/patchew-tester-tmp-ewjgn083/src/linux-user/syscall.c: In
>>>> function ‘do_sendrecvmsg_locked’:
>>>> /var/tmp/patchew-tester-tmp-ewjgn083/src/linux-user/syscall_defs.h:308:61:
>>>> error: ‘tgt_len’ may be used uninitialized in this function
>>>> [-Werror=maybe-uninitialized]
>>>>  #define TARGET_CMSG_LEN(len) (sizeof(struct target_cmsghdr) + 
>>>> (len))
>>>>                                                              
>>>> ^
>>>> /var/tmp/patchew-tester-tmp-ewjgn083/src/linux-user/syscall.c:1797:13: 
>>>> note:
>>>> ‘tgt_len’ was declared here
>>>>          int tgt_len, tgt_space;
>>>>              ^~~~~~~
>>> 
>>> it seems gcc disagrees with Coverity...
>>> 
>>> I think this should fixed like:
>>> 
>>>  diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>>> index 74378947f0..d7fbe334eb 100644
>>> --- a/linux-user/syscall.c
>>> +++ b/linux-user/syscall.c
>>> @@ -1824,8 +1824,10 @@ static inline abi_long 
>>> host_to_target_cmsg(struct
>>> target_msghdr *target_msgh,
>>>                  tgt_len = sizeof(struct 
>>> target_timeval);
>>>                  break;
>>>              default:
>>> +                tgt_len = len;
>>>                  break;
>> 
>> In my view this will result in assigning a wrong value to 
>> ‘tgt_len’ at
>> this ‘switch-case’ condition.
>> Instead looking at the option of initializing ‘tgt_len' to ‘0’.
> 
> According to the comment above the switch():
> 
>         /* Payload types which need a different size of payload on
>          * the target must adjust tgt_len here.
>          */
> 
> So "tgt_len" must be "len" by default, except if it needs to be 
> adjusted
> (currently only for SO_TIMESTAMP), so I don't understand why it should
> be set to "0".
> 
> Thanks,
> Laurent

  1814         switch (cmsg->cmsg_level) {
  1815         case SOL_SOCKET:
  1816             switch (cmsg->cmsg_type) {
  1817             case SO_TIMESTAMP:
  1818                 tgt_len = sizeof(struct target_timeval);
  1819                 break;
  1820             default:
  1821                 break;
  1822             }
  1823         default:
  1824             tgt_len = len;
  1825             break;
  1826         }


If setting tgt_len = len at 1820 then it get set to 'case SOL_SOCKET{ 
switch (cmsg_type's default) }' This place am not sure assingn tgt_len 
with len. To eliminate the gcc uninitialized error thought of 
initializing with '0'.


-- 
Regards,
R.Nageswara Sastry