| Message ID | 20171106132805.19986-1-jfreimann@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 6 November 2017 at 13:28, Jens Freimann <jfreimann@redhat.com> wrote: > This fixes coverity issue CID1005339. > > Make sure that saddr is not used uninitialized if the > mcast parameter is NULL. > > Cc: qemu-stable@nongnu.org > Reported-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Jens Freimann <jfreimann@redhat.com> > --- > net/socket.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/socket.c b/net/socket.c > index e6b471c63d..51eaea67a0 100644 > --- a/net/socket.c > +++ b/net/socket.c > @@ -332,7 +332,7 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer, > const char *mcast, > Error **errp) > { > - struct sockaddr_in saddr; > + struct sockaddr_in saddr = { 0 }; Do we really need the initialization here? With the two if() conditions aligned we should be properly initializing it in all the cases we use it, or have I missed one? thanks -- PMM
Hi Jan, On Mon, Nov 06, 2017 at 02:28:05PM +0100, Jens Freimann wrote: >This fixes coverity issue CID1005339. > >Make sure that saddr is not used uninitialized if the >mcast parameter is NULL. > >Cc: qemu-stable@nongnu.org >Reported-by: Peter Maydell <peter.maydell@linaro.org> >Signed-off-by: Jens Freimann <jfreimann@redhat.com> >--- > net/socket.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > >diff --git a/net/socket.c b/net/socket.c >index e6b471c63d..51eaea67a0 100644 >--- a/net/socket.c >+++ b/net/socket.c >@@ -332,7 +332,7 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer, > const char *mcast, > Error **errp) > { >- struct sockaddr_in saddr; >+ struct sockaddr_in saddr = { 0 }; Based on: https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg04946.html It would appear that the use of {} to initialize the struct is preferred over {0}. Thanks, Darren. > int newfd; > NetClientState *nc; > NetSocketState *s; >@@ -373,7 +373,7 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer, > net_socket_read_poll(s, true); > > /* mcast: save bound address as dst */ >- if (is_connected) { >+ if (is_connected && mcast != NULL) { > s->dgram_dst = saddr; > snprintf(nc->info_str, sizeof(nc->info_str), > "socket: fd=%d (cloned mcast=%s:%d)", >-- >2.13.6 > >
On Mon, Nov 06, 2017 at 01:29:42PM +0000, Peter Maydell wrote: >On 6 November 2017 at 13:28, Jens Freimann <jfreimann@redhat.com> wrote: >> This fixes coverity issue CID1005339. >> >> Make sure that saddr is not used uninitialized if the >> mcast parameter is NULL. >> >> Cc: qemu-stable@nongnu.org >> Reported-by: Peter Maydell <peter.maydell@linaro.org> >> Signed-off-by: Jens Freimann <jfreimann@redhat.com> >> --- >> net/socket.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/net/socket.c b/net/socket.c >> index e6b471c63d..51eaea67a0 100644 >> --- a/net/socket.c >> +++ b/net/socket.c >> @@ -332,7 +332,7 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer, >> const char *mcast, >> Error **errp) >> { >> - struct sockaddr_in saddr; >> + struct sockaddr_in saddr = { 0 }; > >Do we really need the initialization here? With the two if() >conditions aligned we should be properly initializing it >in all the cases we use it, or have I missed one? We don't need it. I added it not to have the same problem again if the code changes in the future. I think it shouldn't hurt because this code is only run once during initialization. If you think it's not necessary I'm fine with removing it though. regards, Jens >thanks >-- PMM
On Mon, Nov 06, 2017 at 01:33:49PM +0000, Darren Kenny wrote: >Hi Jan, > >On Mon, Nov 06, 2017 at 02:28:05PM +0100, Jens Freimann wrote: >>This fixes coverity issue CID1005339. >> >>Make sure that saddr is not used uninitialized if the >>mcast parameter is NULL. >> >>Cc: qemu-stable@nongnu.org >>Reported-by: Peter Maydell <peter.maydell@linaro.org> >>Signed-off-by: Jens Freimann <jfreimann@redhat.com> >>--- >>net/socket.c | 4 ++-- >>1 file changed, 2 insertions(+), 2 deletions(-) >> >>diff --git a/net/socket.c b/net/socket.c >>index e6b471c63d..51eaea67a0 100644 >>--- a/net/socket.c >>+++ b/net/socket.c >>@@ -332,7 +332,7 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer, >> const char *mcast, >> Error **errp) >>{ >>- struct sockaddr_in saddr; >>+ struct sockaddr_in saddr = { 0 }; > >Based on: > > https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg04946.html > >It would appear that the use of {} to initialize the struct is >preferred over {0}. Thanks for the hint Darren. I'll change it in the next version (or get rid of the initalization completely). regards, Jens
On 6 November 2017 at 13:48, Jens Freimann <jfreimann@redhat.com> wrote: > On Mon, Nov 06, 2017 at 01:29:42PM +0000, Peter Maydell wrote: >> Do we really need the initialization here? With the two if() >> conditions aligned we should be properly initializing it >> in all the cases we use it, or have I missed one? > > > We don't need it. I added it not to have the same problem again if > the code changes in the future. I think it shouldn't hurt > because this code is only run once during initialization. The idea is that we want the compiler to tell us if we use this state when it hasn't been properly initialized. Zeroing the variable means that the compiler won't warn, but we'll use zero data, which is unlikely to be the right thing. Occasionally we have to resort to zeroing variables if the compiler can't figure out that we always initialize it if we use it (older gcc versions sometimes produce spurious maybe-used-uninitialized warnings), but we only do that when we have to. thanks -- PMM
On 2017年11月06日 21:28, Jens Freimann wrote: > This fixes coverity issue CID1005339. > > Make sure that saddr is not used uninitialized if the > mcast parameter is NULL. > > Cc: qemu-stable@nongnu.org > Reported-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Jens Freimann <jfreimann@redhat.com> > --- > net/socket.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/socket.c b/net/socket.c > index e6b471c63d..51eaea67a0 100644 > --- a/net/socket.c > +++ b/net/socket.c > @@ -332,7 +332,7 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer, > const char *mcast, > Error **errp) > { > - struct sockaddr_in saddr; > + struct sockaddr_in saddr = { 0 }; > int newfd; > NetClientState *nc; > NetSocketState *s; > @@ -373,7 +373,7 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer, > net_socket_read_poll(s, true); > > /* mcast: save bound address as dst */ > - if (is_connected) { > + if (is_connected && mcast != NULL) { > s->dgram_dst = saddr; > snprintf(nc->info_str, sizeof(nc->info_str), > "socket: fd=%d (cloned mcast=%s:%d)", Applied, thanks.
On 13 November 2017 at 07:13, Jason Wang <jasowang@redhat.com> wrote: > > > On 2017年11月06日 21:28, Jens Freimann wrote: >> >> This fixes coverity issue CID1005339. >> >> Make sure that saddr is not used uninitialized if the >> mcast parameter is NULL. >> >> Cc: qemu-stable@nongnu.org >> Reported-by: Peter Maydell <peter.maydell@linaro.org> >> Signed-off-by: Jens Freimann <jfreimann@redhat.com> >> --- >> net/socket.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/net/socket.c b/net/socket.c >> index e6b471c63d..51eaea67a0 100644 >> --- a/net/socket.c >> +++ b/net/socket.c >> @@ -332,7 +332,7 @@ static NetSocketState >> *net_socket_fd_init_dgram(NetClientState *peer, >> const char *mcast, >> Error **errp) >> { >> - struct sockaddr_in saddr; >> + struct sockaddr_in saddr = { 0 }; >> int newfd; >> NetClientState *nc; >> NetSocketState *s; >> @@ -373,7 +373,7 @@ static NetSocketState >> *net_socket_fd_init_dgram(NetClientState *peer, >> net_socket_read_poll(s, true); >> /* mcast: save bound address as dst */ >> - if (is_connected) { >> + if (is_connected && mcast != NULL) { >> s->dgram_dst = saddr; >> snprintf(nc->info_str, sizeof(nc->info_str), >> "socket: fd=%d (cloned mcast=%s:%d)", > > > Applied, thanks. Er, this version didn't pass code review and you should apply v2 instead... thanks -- PMM
On 2017年11月13日 17:51, Peter Maydell wrote: > On 13 November 2017 at 07:13, Jason Wang <jasowang@redhat.com> wrote: >> >> On 2017年11月06日 21:28, Jens Freimann wrote: >>> This fixes coverity issue CID1005339. >>> >>> Make sure that saddr is not used uninitialized if the >>> mcast parameter is NULL. >>> >>> Cc: qemu-stable@nongnu.org >>> Reported-by: Peter Maydell <peter.maydell@linaro.org> >>> Signed-off-by: Jens Freimann <jfreimann@redhat.com> >>> --- >>> net/socket.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/net/socket.c b/net/socket.c >>> index e6b471c63d..51eaea67a0 100644 >>> --- a/net/socket.c >>> +++ b/net/socket.c >>> @@ -332,7 +332,7 @@ static NetSocketState >>> *net_socket_fd_init_dgram(NetClientState *peer, >>> const char *mcast, >>> Error **errp) >>> { >>> - struct sockaddr_in saddr; >>> + struct sockaddr_in saddr = { 0 }; >>> int newfd; >>> NetClientState *nc; >>> NetSocketState *s; >>> @@ -373,7 +373,7 @@ static NetSocketState >>> *net_socket_fd_init_dgram(NetClientState *peer, >>> net_socket_read_poll(s, true); >>> /* mcast: save bound address as dst */ >>> - if (is_connected) { >>> + if (is_connected && mcast != NULL) { >>> s->dgram_dst = saddr; >>> snprintf(nc->info_str, sizeof(nc->info_str), >>> "socket: fd=%d (cloned mcast=%s:%d)", >> >> Applied, thanks. > Er, this version didn't pass code review and you should > apply v2 instead... > > thanks > -- PMM Oops, indeed. Apply V2. Thanks
diff --git a/net/socket.c b/net/socket.c index e6b471c63d..51eaea67a0 100644 --- a/net/socket.c +++ b/net/socket.c @@ -332,7 +332,7 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer, const char *mcast, Error **errp) { - struct sockaddr_in saddr; + struct sockaddr_in saddr = { 0 }; int newfd; NetClientState *nc; NetSocketState *s; @@ -373,7 +373,7 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer, net_socket_read_poll(s, true); /* mcast: save bound address as dst */ - if (is_connected) { + if (is_connected && mcast != NULL) { s->dgram_dst = saddr; snprintf(nc->info_str, sizeof(nc->info_str), "socket: fd=%d (cloned mcast=%s:%d)",
This fixes coverity issue CID1005339. Make sure that saddr is not used uninitialized if the mcast parameter is NULL. Cc: qemu-stable@nongnu.org Reported-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Jens Freimann <jfreimann@redhat.com> --- net/socket.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)