diff mbox series

[V3] net/colo-compare.c: Fix memory leak and code style issue.

Message ID 20190718092219.20081-1-chen.zhang@intel.com
State New
Headers show
Series [V3] net/colo-compare.c: Fix memory leak and code style issue. | expand

Commit Message

Zhang, Chen July 18, 2019, 9:22 a.m. UTC 
From: Zhang Chen <chen.zhang@intel.com>

This patch to fix the origin "char *data" menory leak, code style issue
and add necessary check here.
Reported-by: Coverity (CID 1402785)

Signed-off-by: Zhang Chen <chen.zhang@intel.com>
---
 net/colo-compare.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

Comments

Philippe Mathieu-Daudé July 18, 2019, 10:28 a.m. UTC | #1 
On 7/18/19 11:22 AM, Zhang Chen wrote:
> From: Zhang Chen <chen.zhang@intel.com>
> 
> This patch to fix the origin "char *data" menory leak, code style issue

"memory"

> and add necessary check here.
> Reported-by: Coverity (CID 1402785)
> 
> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> ---
>  net/colo-compare.c | 28 +++++++++++++++++++++-------
>  1 file changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index 909dd6c6eb..fcccb4c6f6 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -127,6 +127,17 @@ static int compare_chr_send(CompareState *s,
>                              uint32_t vnet_hdr_len,
>                              bool notify_remote_frame);
>  
> +static bool packet_matches_str(const char *str,
> +                               uint8_t *buf,

You can use 'uint8_t *buf'.

> +                               uint32_t packet_len)
> +{
> +    if (packet_len != strlen(str)) {
> +        return false;
> +    }
> +
> +    return !memcmp(str, buf, strlen(str));

If you don't want to use a local variable to hold strlen(str), you can
reuse packet_len since it is the same value:

       return !memcmp(str, buf, packet_len);

However it makes the review harder, so I'd prefer using a str_len local var.

> +}
> +
>  static void notify_remote_frame(CompareState *s)
>  {
>      char msg[] = "DO_CHECKPOINT";
> @@ -1008,21 +1019,24 @@ static void compare_notify_rs_finalize(SocketReadState *notify_rs)
>  {
>      CompareState *s = container_of(notify_rs, CompareState, notify_rs);
>  
> -    /* Get Xen colo-frame's notify and handle the message */
> -    char *data = g_memdup(notify_rs->buf, notify_rs->packet_len);
> -    char msg[] = "COLO_COMPARE_GET_XEN_INIT";
> +    const char msg[] = "COLO_COMPARE_GET_XEN_INIT";
>      int ret;
>  
> -    if (!strcmp(data, "COLO_USERSPACE_PROXY_INIT")) {
> +    if (packet_matches_str("COLO_USERSPACE_PROXY_INIT",
> +                           notify_rs->buf,
> +                           notify_rs->packet_len)) {
>          ret = compare_chr_send(s, (uint8_t *)msg, strlen(msg), 0, true);
>          if (ret < 0) {
>              error_report("Notify Xen COLO-frame INIT failed");
>          }
> -    }
> -
> -    if (!strcmp(data, "COLO_CHECKPOINT")) {
> +    } else if (packet_matches_str("COLO_CHECKPOINT",
> +                                  notify_rs->buf,
> +                                  notify_rs->packet_len)) {
>          /* colo-compare do checkpoint, flush pri packet and remove sec packet */
>          g_queue_foreach(&s->conn_list, colo_flush_packets, s);
> +    } else {
> +        error_report("COLO compare got unsupported instruction '%s'",
> +                     (char *)notify_rs->buf);
>      }
>  }
>  
>
Peter Maydell July 18, 2019, 10:37 a.m. UTC | #2 
On Thu, 18 Jul 2019 at 11:28, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> On 7/18/19 11:22 AM, Zhang Chen wrote:
> > From: Zhang Chen <chen.zhang@intel.com>
> >
> > This patch to fix the origin "char *data" menory leak, code style issue
>
> "memory"
>
> > and add necessary check here.
> > Reported-by: Coverity (CID 1402785)
> >
> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> > ---
> >  net/colo-compare.c | 28 +++++++++++++++++++++-------
> >  1 file changed, 21 insertions(+), 7 deletions(-)
> >
> > diff --git a/net/colo-compare.c b/net/colo-compare.c
> > index 909dd6c6eb..fcccb4c6f6 100644
> > --- a/net/colo-compare.c
> > +++ b/net/colo-compare.c
> > @@ -127,6 +127,17 @@ static int compare_chr_send(CompareState *s,
> >                              uint32_t vnet_hdr_len,
> >                              bool notify_remote_frame);
> >
> > +static bool packet_matches_str(const char *str,
> > +                               uint8_t *buf,
>
> You can use 'uint8_t *buf'.

?? that seems to be the same as what is written...

>
> > +                               uint32_t packet_len)
> > +{
> > +    if (packet_len != strlen(str)) {
> > +        return false;
> > +    }
> > +
> > +    return !memcmp(str, buf, strlen(str));
>
> If you don't want to use a local variable to hold strlen(str), you can
> reuse packet_len since it is the same value:
>
>        return !memcmp(str, buf, packet_len);
>
> However it makes the review harder, so I'd prefer using a str_len local var.

I'm pretty sure the compiler is going to optimise the
strlen() away into a compile time constant anyway, so
this is somewhat unnecessary micro-optimisation I think.

thanks
-- PMM
Philippe Mathieu-Daudé July 18, 2019, 10:53 a.m. UTC | #3 
On 7/18/19 12:37 PM, Peter Maydell wrote:
> On Thu, 18 Jul 2019 at 11:28, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> On 7/18/19 11:22 AM, Zhang Chen wrote:
>>> From: Zhang Chen <chen.zhang@intel.com>
>>>
>>> This patch to fix the origin "char *data" menory leak, code style issue
>>
>> "memory"
>>
>>> and add necessary check here.
>>> Reported-by: Coverity (CID 1402785)
>>>
>>> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
>>> ---
>>>  net/colo-compare.c | 28 +++++++++++++++++++++-------
>>>  1 file changed, 21 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/net/colo-compare.c b/net/colo-compare.c
>>> index 909dd6c6eb..fcccb4c6f6 100644
>>> --- a/net/colo-compare.c
>>> +++ b/net/colo-compare.c
>>> @@ -127,6 +127,17 @@ static int compare_chr_send(CompareState *s,
>>>                              uint32_t vnet_hdr_len,
>>>                              bool notify_remote_frame);
>>>
>>> +static bool packet_matches_str(const char *str,
>>> +                               uint8_t *buf,
>>
>> You can use 'uint8_t *buf'.
> 
> ?? that seems to be the same as what is written...

Oops sorry, I copy/pasted and  did not noticed I removed the 'const'
word. So I meant: You can use 'const uint8_t *buf'

>>
>>> +                               uint32_t packet_len)
>>> +{
>>> +    if (packet_len != strlen(str)) {
>>> +        return false;
>>> +    }
>>> +
>>> +    return !memcmp(str, buf, strlen(str));
>>
>> If you don't want to use a local variable to hold strlen(str), you can
>> reuse packet_len since it is the same value:
>>
>>        return !memcmp(str, buf, packet_len);
>>
>> However it makes the review harder, so I'd prefer using a str_len local var.
> 
> I'm pretty sure the compiler is going to optimise the
> strlen() away into a compile time constant anyway, so
> this is somewhat unnecessary micro-optimisation I think.

I was not sure, I'm glad to learn that :)

Thanks,

Phil.
Peter Maydell July 18, 2019, 1:42 p.m. UTC | #4 
On Thu, 18 Jul 2019 at 10:27, Zhang Chen <chen.zhang@intel.com> wrote:
>
> From: Zhang Chen <chen.zhang@intel.com>
>
> This patch to fix the origin "char *data" menory leak, code style issue
> and add necessary check here.
> Reported-by: Coverity (CID 1402785)
>
> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> ---
>  net/colo-compare.c | 28 +++++++++++++++++++++-------
>  1 file changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index 909dd6c6eb..fcccb4c6f6 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -127,6 +127,17 @@ static int compare_chr_send(CompareState *s,
>                              uint32_t vnet_hdr_len,
>                              bool notify_remote_frame);
>
> +static bool packet_matches_str(const char *str,
> +                               uint8_t *buf,
> +                               uint32_t packet_len)
> +{
> +    if (packet_len != strlen(str)) {

Is '!=' definitely correct? (ie the incoming packet must
*not* contain a trailing '\0' or any other trailing data) ?

Is there a specification of the protocol somewhere? If
so, that presumably should say one way or the other.

> +        return false;
> +    }
> +
> +    return !memcmp(str, buf, strlen(str));
> +}

thanks
-- PMM
Zhang, Chen July 19, 2019, 1:54 a.m. UTC | #5 
> -----Original Message-----
> From: Peter Maydell [mailto:peter.maydell@linaro.org]
> Sent: Thursday, July 18, 2019 9:42 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: Li Zhijian <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>;
> qemu-dev <qemu-devel@nongnu.org>; Zhang Chen <zhangckid@gmail.com>
> Subject: Re: [PATCH V3] net/colo-compare.c: Fix memory leak and code style
> issue.
> 
> On Thu, 18 Jul 2019 at 10:27, Zhang Chen <chen.zhang@intel.com> wrote:
> >
> > From: Zhang Chen <chen.zhang@intel.com>
> >
> > This patch to fix the origin "char *data" menory leak, code style
> > issue and add necessary check here.
> > Reported-by: Coverity (CID 1402785)
> >
> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> > ---
> >  net/colo-compare.c | 28 +++++++++++++++++++++-------
> >  1 file changed, 21 insertions(+), 7 deletions(-)
> >
> > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > 909dd6c6eb..fcccb4c6f6 100644
> > --- a/net/colo-compare.c
> > +++ b/net/colo-compare.c
> > @@ -127,6 +127,17 @@ static int compare_chr_send(CompareState *s,
> >                              uint32_t vnet_hdr_len,
> >                              bool notify_remote_frame);
> >
> > +static bool packet_matches_str(const char *str,
> > +                               uint8_t *buf,
> > +                               uint32_t packet_len) {
> > +    if (packet_len != strlen(str)) {
> 
> Is '!=' definitely correct? (ie the incoming packet must
> *not* contain a trailing '\0' or any other trailing data) ?

Yes, the packet not contain a trail.
As Jason comments before, you can see the net/net.c  "net_fill_rstate()".
We just got the length and data.

Thanks
Zhang Chen

> 
> Is there a specification of the protocol somewhere? If so, that presumably
> should say one way or the other.
> 
> > +        return false;
> > +    }
> > +
> > +    return !memcmp(str, buf, strlen(str)); }
> 
> thanks
> -- PMM
Zhang, Chen July 19, 2019, 1:56 a.m. UTC | #6 
> -----Original Message-----
> From: Philippe Mathieu-Daudé [mailto:philmd@redhat.com]
> Sent: Thursday, July 18, 2019 6:54 PM
> To: Peter Maydell <peter.maydell@linaro.org>
> Cc: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian <lizhijian@cn.fujitsu.com>;
> Jason Wang <jasowang@redhat.com>; qemu-dev <qemu-devel@nongnu.org>;
> Zhang Chen <zhangckid@gmail.com>
> Subject: Re: [Qemu-devel] [PATCH V3] net/colo-compare.c: Fix memory leak
> and code style issue.
> 
> On 7/18/19 12:37 PM, Peter Maydell wrote:
> > On Thu, 18 Jul 2019 at 11:28, Philippe Mathieu-Daudé <philmd@redhat.com>
> wrote:
> >>
> >> On 7/18/19 11:22 AM, Zhang Chen wrote:
> >>> From: Zhang Chen <chen.zhang@intel.com>
> >>>
> >>> This patch to fix the origin "char *data" menory leak, code style
> >>> issue
> >>
> >> "memory"

Oh, I will fix this typo in next version.

> >>
> >>> and add necessary check here.
> >>> Reported-by: Coverity (CID 1402785)
> >>>
> >>> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> >>> ---
> >>>  net/colo-compare.c | 28 +++++++++++++++++++++-------
> >>>  1 file changed, 21 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/net/colo-compare.c b/net/colo-compare.c index
> >>> 909dd6c6eb..fcccb4c6f6 100644
> >>> --- a/net/colo-compare.c
> >>> +++ b/net/colo-compare.c
> >>> @@ -127,6 +127,17 @@ static int compare_chr_send(CompareState *s,
> >>>                              uint32_t vnet_hdr_len,
> >>>                              bool notify_remote_frame);
> >>>
> >>> +static bool packet_matches_str(const char *str,
> >>> +                               uint8_t *buf,
> >>
> >> You can use 'uint8_t *buf'.
> >
> > ?? that seems to be the same as what is written...
> 
> Oops sorry, I copy/pasted and  did not noticed I removed the 'const'
> word. So I meant: You can use 'const uint8_t *buf'

OK.

Thanks
Zhang Chen

> 
> >>
> >>> +                               uint32_t packet_len) {
> >>> +    if (packet_len != strlen(str)) {
> >>> +        return false;
> >>> +    }
> >>> +
> >>> +    return !memcmp(str, buf, strlen(str));
> >>
> >> If you don't want to use a local variable to hold strlen(str), you
> >> can reuse packet_len since it is the same value:
> >>
> >>        return !memcmp(str, buf, packet_len);
> >>
> >> However it makes the review harder, so I'd prefer using a str_len local var.
> >
> > I'm pretty sure the compiler is going to optimise the
> > strlen() away into a compile time constant anyway, so this is somewhat
> > unnecessary micro-optimisation I think.
> 
> I was not sure, I'm glad to learn that :)
> 
> Thanks,
> 
> Phil.
Zhang, Chen July 21, 2019, 9:07 a.m. UTC | #7 
Sorry, I re-sent the old version.
Please redirect to V4 patch.

Thanks
Zhang Chen

> -----Original Message-----
> From: Zhang, Chen
> Sent: Thursday, July 18, 2019 5:22 PM
> To: Li Zhijian <lizhijian@cn.fujitsu.com>; Peter Maydell
> <peter.maydell@linaro.org>; Jason Wang <jasowang@redhat.com>; qemu-dev
> <qemu-devel@nongnu.org>
> Cc: Zhang Chen <zhangckid@gmail.com>; Zhang, Chen <chen.zhang@intel.com>
> Subject: [PATCH V3] net/colo-compare.c: Fix memory leak and code style issue.
> 
> From: Zhang Chen <chen.zhang@intel.com>
> 
> This patch to fix the origin "char *data" menory leak, code style issue and add
> necessary check here.
> Reported-by: Coverity (CID 1402785)
> 
> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> ---
>  net/colo-compare.c | 28 +++++++++++++++++++++-------
>  1 file changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/net/colo-compare.c b/net/colo-compare.c index
> 909dd6c6eb..fcccb4c6f6 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -127,6 +127,17 @@ static int compare_chr_send(CompareState *s,
>                              uint32_t vnet_hdr_len,
>                              bool notify_remote_frame);
> 
> +static bool packet_matches_str(const char *str,
> +                               uint8_t *buf,
> +                               uint32_t packet_len) {
> +    if (packet_len != strlen(str)) {
> +        return false;
> +    }
> +
> +    return !memcmp(str, buf, strlen(str)); }
> +
>  static void notify_remote_frame(CompareState *s)  {
>      char msg[] = "DO_CHECKPOINT";
> @@ -1008,21 +1019,24 @@ static void
> compare_notify_rs_finalize(SocketReadState *notify_rs)  {
>      CompareState *s = container_of(notify_rs, CompareState, notify_rs);
> 
> -    /* Get Xen colo-frame's notify and handle the message */
> -    char *data = g_memdup(notify_rs->buf, notify_rs->packet_len);
> -    char msg[] = "COLO_COMPARE_GET_XEN_INIT";
> +    const char msg[] = "COLO_COMPARE_GET_XEN_INIT";
>      int ret;
> 
> -    if (!strcmp(data, "COLO_USERSPACE_PROXY_INIT")) {
> +    if (packet_matches_str("COLO_USERSPACE_PROXY_INIT",
> +                           notify_rs->buf,
> +                           notify_rs->packet_len)) {
>          ret = compare_chr_send(s, (uint8_t *)msg, strlen(msg), 0, true);
>          if (ret < 0) {
>              error_report("Notify Xen COLO-frame INIT failed");
>          }
> -    }
> -
> -    if (!strcmp(data, "COLO_CHECKPOINT")) {
> +    } else if (packet_matches_str("COLO_CHECKPOINT",
> +                                  notify_rs->buf,
> +                                  notify_rs->packet_len)) {
>          /* colo-compare do checkpoint, flush pri packet and remove sec packet */
>          g_queue_foreach(&s->conn_list, colo_flush_packets, s);
> +    } else {
> +        error_report("COLO compare got unsupported instruction '%s'",
> +                     (char *)notify_rs->buf);
>      }
>  }
> 
> --
> 2.17.GIT
diff mbox series

Patch

diff --git a/net/colo-compare.c b/net/colo-compare.c
index 909dd6c6eb..fcccb4c6f6 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -127,6 +127,17 @@  static int compare_chr_send(CompareState *s,
                             uint32_t vnet_hdr_len,
                             bool notify_remote_frame);
 
+static bool packet_matches_str(const char *str,
+                               uint8_t *buf,
+                               uint32_t packet_len)
+{
+    if (packet_len != strlen(str)) {
+        return false;
+    }
+
+    return !memcmp(str, buf, strlen(str));
+}
+
 static void notify_remote_frame(CompareState *s)
 {
     char msg[] = "DO_CHECKPOINT";
@@ -1008,21 +1019,24 @@  static void compare_notify_rs_finalize(SocketReadState *notify_rs)
 {
     CompareState *s = container_of(notify_rs, CompareState, notify_rs);
 
-    /* Get Xen colo-frame's notify and handle the message */
-    char *data = g_memdup(notify_rs->buf, notify_rs->packet_len);
-    char msg[] = "COLO_COMPARE_GET_XEN_INIT";
+    const char msg[] = "COLO_COMPARE_GET_XEN_INIT";
     int ret;
 
-    if (!strcmp(data, "COLO_USERSPACE_PROXY_INIT")) {
+    if (packet_matches_str("COLO_USERSPACE_PROXY_INIT",
+                           notify_rs->buf,
+                           notify_rs->packet_len)) {
         ret = compare_chr_send(s, (uint8_t *)msg, strlen(msg), 0, true);
         if (ret < 0) {
             error_report("Notify Xen COLO-frame INIT failed");
         }
-    }
-
-    if (!strcmp(data, "COLO_CHECKPOINT")) {
+    } else if (packet_matches_str("COLO_CHECKPOINT",
+                                  notify_rs->buf,
+                                  notify_rs->packet_len)) {
         /* colo-compare do checkpoint, flush pri packet and remove sec packet */
         g_queue_foreach(&s->conn_list, colo_flush_packets, s);
+    } else {
+        error_report("COLO compare got unsupported instruction '%s'",
+                     (char *)notify_rs->buf);
     }
 }