[Qemu-devel] [PATCH] net/socket: fix coverity issue

[Qemu-devel] [PATCH] net/socket: fix coverity issue

[Jens Freimann]

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)",
-- 
2.13.6

Re: [Qemu-devel] [PATCH] net/socket: fix coverity issue

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] net/socket: fix coverity issue

[Jens Freimann]

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

Re: [Qemu-devel] [PATCH] net/socket: fix coverity issue

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] net/socket: fix coverity issue

[Darren Kenny]

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
>
>

Re: [Qemu-devel] [PATCH] net/socket: fix coverity issue

[Jens Freimann]

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 

Re: [Qemu-devel] [PATCH] net/socket: fix coverity issue

[Jason Wang]

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.

Re: [Qemu-devel] [PATCH] net/socket: fix coverity issue

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] net/socket: fix coverity issue

[Jason Wang]

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