Message ID | 20190704083647.30614-1-chen.zhang@intel.com |
---|---|
State | New |
Headers | show |
Series | [V2] net/colo-compare.c: Fix memory leak and code style issue. | expand |
On 2019/7/4 下午4:36, Zhang Chen 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..ed349f5f6a 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) + 1); This assumes buf is NULL terminated (you pass notify_rs->buf) which is not correct I think? Thanks > +} > + > 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); > } > } >
> -----Original Message----- > From: Jason Wang [mailto:jasowang@redhat.com] > Sent: Tuesday, July 9, 2019 10:48 PM > To: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian <lizhijian@cn.fujitsu.com>; > Peter Maydell <peter.maydell@linaro.org>; qemu-dev <qemu- > devel@nongnu.org> > Cc: Zhang Chen <zhangckid@gmail.com> > Subject: Re: [Qemu-devel] [PATCH V2] net/colo-compare.c: Fix memory leak > and code style issue. > > > On 2019/7/4 下午4:36, Zhang Chen 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..ed349f5f6a 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) + 1); > > > This assumes buf is NULL terminated (you pass notify_rs->buf) which is not > correct I think? Yes, you are right. How about this: static bool packet_matches_str(const char *str, uint8_t *buf, uint32_t packet_len) { if (packet_len != strlen(str) || !buf) { return false; } return !memcmp(str, buf, strlen(str)); } Thanks Zhang Chen > > Thanks > > > > +} > > + > > 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); > > } > > } > >
On 2019/7/10 下午3:50, Zhang, Chen wrote: > >> -----Original Message----- >> From: Jason Wang [mailto:jasowang@redhat.com] >> Sent: Tuesday, July 9, 2019 10:48 PM >> To: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian <lizhijian@cn.fujitsu.com>; >> Peter Maydell <peter.maydell@linaro.org>; qemu-dev <qemu- >> devel@nongnu.org> >> Cc: Zhang Chen <zhangckid@gmail.com> >> Subject: Re: [Qemu-devel] [PATCH V2] net/colo-compare.c: Fix memory leak >> and code style issue. >> >> >> On 2019/7/4 下午4:36, Zhang Chen 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..ed349f5f6a 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) + 1); >> >> This assumes buf is NULL terminated (you pass notify_rs->buf) which is not >> correct I think? > Yes, you are right. > How about this: > > static bool packet_matches_str(const char *str, > uint8_t *buf, > uint32_t packet_len) { > if (packet_len != strlen(str) || !buf) { > return false; > } When can we hit !buf? Thanks > > return !memcmp(str, buf, strlen(str)); > } > > Thanks > Zhang Chen > > >> Thanks >> >> >>> +} >>> + >>> 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); >>> } >>> } >>>
> -----Original Message----- > From: Jason Wang [mailto:jasowang@redhat.com] > Sent: Thursday, July 11, 2019 4:18 AM > To: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian <lizhijian@cn.fujitsu.com>; > Peter Maydell <peter.maydell@linaro.org>; qemu-dev <qemu- > devel@nongnu.org> > Cc: Zhang Chen <zhangckid@gmail.com> > Subject: Re: [Qemu-devel] [PATCH V2] net/colo-compare.c: Fix memory leak > and code style issue. > > > On 2019/7/10 下午3:50, Zhang, Chen wrote: > > > >> -----Original Message----- > >> From: Jason Wang [mailto:jasowang@redhat.com] > >> Sent: Tuesday, July 9, 2019 10:48 PM > >> To: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian > >> <lizhijian@cn.fujitsu.com>; Peter Maydell <peter.maydell@linaro.org>; > >> qemu-dev <qemu- devel@nongnu.org> > >> Cc: Zhang Chen <zhangckid@gmail.com> > >> Subject: Re: [Qemu-devel] [PATCH V2] net/colo-compare.c: Fix memory > >> leak and code style issue. > >> > >> > >> On 2019/7/4 下午4:36, Zhang Chen 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..ed349f5f6a 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) + 1); > >> > >> This assumes buf is NULL terminated (you pass notify_rs->buf) which > >> is not correct I think? > > Yes, you are right. > > How about this: > > > > static bool packet_matches_str(const char *str, > > uint8_t *buf, > > uint32_t packet_len) { > > if (packet_len != strlen(str) || !buf) { > > return false; > > } > > > When can we hit !buf? I re-checked the code, looks the "net_fill_rstate()" can ensure buf is not empty, I will remove the !buf to send next version. Thanks Zhang Chen > > Thanks > > > > > > return !memcmp(str, buf, strlen(str)); } > > > > Thanks > > Zhang Chen > > > > > >> Thanks > >> > >> > >>> +} > >>> + > >>> 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); > >>> } > >>> } > >>>
diff --git a/net/colo-compare.c b/net/colo-compare.c index 909dd6c6eb..ed349f5f6a 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) + 1); +} + 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); } }