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

[Nageswara R Sastry]

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, address@hidden 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