[Qemu-devel] [PATCH 0/3] qdist fixes

[Qemu-devel] [PATCH 0/3] qdist fixes

[Emilio G. Cota]

While fixing the return of a NULL string when printing an empty
dist (patch 3) (see background here [*]), I noticed there was
a leak in qdist (patch 1). Patch 2 is trivial.

Thanks,

		Emilio

[*] https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg05494.html

[Qemu-devel] [PATCH 1/3] qdist: fix memory leak during binning

[Emilio G. Cota]

In qdist_bin__internal(), to->entries is initialized to a 1-element array,
which we then leak when n == from->n. Fix it.

Signed-off-by: Emilio G. Cota <cota@braap.org>
---
 util/qdist.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/qdist.c b/util/qdist.c
index 56f5738..eb2236c 100644
--- a/util/qdist.c
+++ b/util/qdist.c
@@ -188,7 +188,7 @@ void qdist_bin__internal(struct qdist *to, const struct qdist *from, size_t n)
             }
         }
         /* they're equally spaced, so copy the dist and bail out */
-        to->entries = g_new(struct qdist_entry, from->n);
+        to->entries = g_realloc_n(to->entries, n, sizeof(*to->entries));
         to->n = from->n;
         memcpy(to->entries, from->entries, sizeof(*to->entries) * to->n);
         return;
-- 
2.5.0

Re: [Qemu-devel] [PATCH 1/3] qdist: fix memory leak during binning

[Marc-André Lureau]

Hi

On Mon, Jul 25, 2016 at 7:03 PM, Emilio G. Cota <cota@braap.org> wrote:
> In qdist_bin__internal(), to->entries is initialized to a 1-element array,
> which we then leak when n == from->n. Fix it.
>
> Signed-off-by: Emilio G. Cota <cota@braap.org>
> ---
>  util/qdist.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/util/qdist.c b/util/qdist.c
> index 56f5738..eb2236c 100644
> --- a/util/qdist.c
> +++ b/util/qdist.c
> @@ -188,7 +188,7 @@ void qdist_bin__internal(struct qdist *to, const struct qdist *from, size_t n)
>              }
>          }
>          /* they're equally spaced, so copy the dist and bail out */
> -        to->entries = g_new(struct qdist_entry, from->n);
> +        to->entries = g_realloc_n(to->entries, n, sizeof(*to->entries));

This is already part of the leak series:
https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg04168.html

>          to->n = from->n;
>          memcpy(to->entries, from->entries, sizeof(*to->entries) * to->n);
>          return;
> --
> 2.5.0
>
>



-- 
Marc-André Lureau

[Qemu-devel] [PATCH 2/3] qdist: use g_realloc_n instead of g_realloc

[Emilio G. Cota]

While at it, remove the unnecessary parentheses around dist->size.

Signed-off-by: Emilio G. Cota <cota@braap.org>
---
 util/qdist.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/util/qdist.c b/util/qdist.c
index eb2236c..cc31140 100644
--- a/util/qdist.c
+++ b/util/qdist.c
@@ -62,8 +62,8 @@ void qdist_add(struct qdist *dist, double x, long count)
 
     if (unlikely(dist->n == dist->size)) {
         dist->size *= 2;
-        dist->entries = g_realloc(dist->entries,
-                                  sizeof(*dist->entries) * (dist->size));
+        dist->entries = g_realloc_n(dist->entries, dist->size,
+                                    sizeof(*dist->entries));
     }
     dist->n++;
     entry = &dist->entries[dist->n - 1];
-- 
2.5.0

Re: [Qemu-devel] [PATCH 2/3] qdist: use g_realloc_n instead of g_realloc

[Marc-André Lureau]

Hi

On Mon, Jul 25, 2016 at 7:03 PM, Emilio G. Cota <cota@braap.org> wrote:
> While at it, remove the unnecessary parentheses around dist->size.
>
> Signed-off-by: Emilio G. Cota <cota@braap.org>
> ---
>  util/qdist.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/util/qdist.c b/util/qdist.c
> index eb2236c..cc31140 100644
> --- a/util/qdist.c
> +++ b/util/qdist.c
> @@ -62,8 +62,8 @@ void qdist_add(struct qdist *dist, double x, long count)
>
>      if (unlikely(dist->n == dist->size)) {
>          dist->size *= 2;
> -        dist->entries = g_realloc(dist->entries,
> -                                  sizeof(*dist->entries) * (dist->size));
> +        dist->entries = g_realloc_n(dist->entries, dist->size,
> +                                    sizeof(*dist->entries));
>      }
>      dist->n++;
>      entry = &dist->entries[dist->n - 1];
> --
> 2.5.0
>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>



-- 
Marc-André Lureau

[Qemu-devel] [PATCH 3/3] qdist: return "(empty)" instead of NULL when printing an empty dist

[Emilio G. Cota]

Printf'ing a NULL string is undefined behaviour. Avoid it.

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Emilio G. Cota <cota@braap.org>
---
 tests/test-qdist.c | 10 ++++++++--
 util/qdist.c       |  6 ++++--
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/tests/test-qdist.c b/tests/test-qdist.c
index 0298986..9541ce3 100644
--- a/tests/test-qdist.c
+++ b/tests/test-qdist.c
@@ -360,10 +360,16 @@ static void test_none(void)
     g_assert(isnan(qdist_xmax(&dist)));
 
     pr = qdist_pr_plain(&dist, 0);
-    g_assert(pr == NULL);
+    g_assert_cmpstr(pr, ==, "(empty)");
+    g_free(pr);
 
     pr = qdist_pr_plain(&dist, 2);
-    g_assert(pr == NULL);
+    g_assert_cmpstr(pr, ==, "(empty)");
+    g_free(pr);
+
+    pr = qdist_pr(&dist, 0, QDIST_PR_BORDER);
+    g_assert_cmpstr(pr, ==, "(empty)");
+    g_free(pr);
 
     qdist_destroy(&dist);
 }
diff --git a/util/qdist.c b/util/qdist.c
index cc31140..41eff08 100644
--- a/util/qdist.c
+++ b/util/qdist.c
@@ -14,6 +14,8 @@
 #define NAN (0.0 / 0.0)
 #endif
 
+#define QDIST_EMPTY_STR "(empty)"
+
 void qdist_init(struct qdist *dist)
 {
     dist->entries = g_malloc(sizeof(*dist->entries));
@@ -234,7 +236,7 @@ char *qdist_pr_plain(const struct qdist *dist, size_t n)
     char *ret;
 
     if (dist->n == 0) {
-        return NULL;
+        return g_strdup(QDIST_EMPTY_STR);
     }
     qdist_bin__internal(&binned, dist, n);
     ret = qdist_pr_internal(&binned);
@@ -309,7 +311,7 @@ char *qdist_pr(const struct qdist *dist, size_t n_bins, uint32_t opt)
     GString *s;
 
     if (dist->n == 0) {
-        return NULL;
+        return g_strdup(QDIST_EMPTY_STR);
     }
 
     s = g_string_new("");
-- 
2.5.0

Re: [Qemu-devel] [PATCH 0/3] qdist fixes

[Paolo Bonzini]

On 25/07/2016 17:03, Emilio G. Cota wrote:
> While fixing the return of a NULL string when printing an empty
> dist (patch 3) (see background here [*]), I noticed there was
> a leak in qdist (patch 1). Patch 2 is trivial.
> 
> Thanks,
> 
> 		Emilio
> 
> [*] https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg05494.html
> 

Queued all, thanks.

Paolo