From 1ac8d50331d3b32390ac8a8e35e1ad860a7b4c3e Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 16 Jul 2019 10:15:09 +0100 Subject: [PATCH 1/7] gptrarray: Set free func on copied array in g_ptr_array_copy() Otherwise its elements (which have just all been copied) will leak. Spotted by Xavier Claessens in https://gitlab.gnome.org/GNOME/glib/merge_requests/918#note_555867. Signed-off-by: Philip Withnall --- glib/garray.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/glib/garray.c b/glib/garray.c index 29be5f7ef..075cb8d73 100644 --- a/glib/garray.c +++ b/glib/garray.c @@ -1020,6 +1020,9 @@ g_ptr_array_new (void) * If @func is %NULL, then only the pointers (and not what they are * pointing to) are copied to the new #GPtrArray. * + * The copy of @array will have the same #GDestroyNotify for its elements as + * @array. + * * Returns: (transfer full): a deep copy of the initial #GPtrArray. * * Since: 2.62 @@ -1035,6 +1038,8 @@ g_ptr_array_copy (GPtrArray *array, g_return_val_if_fail (array != NULL, NULL); new_array = g_ptr_array_sized_new (array->len); + g_ptr_array_set_free_func (new_array, ((GRealPtrArray *) array)->element_free_func); + if (func != NULL) { for (i = 0; i < array->len; i++) From b4943aa360f9db6f5319b14f077b7a071686d35e Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 16 Jul 2019 10:15:57 +0100 Subject: [PATCH 2/7] gptrarray: Correctly set copied array length in g_ptr_array_copy() The allocation size was set correctly before, but not the array length, so the copied array appeared to have zero elements. Signed-off-by: Philip Withnall --- glib/garray.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/glib/garray.c b/glib/garray.c index 075cb8d73..6e70f859e 100644 --- a/glib/garray.c +++ b/glib/garray.c @@ -1051,6 +1051,8 @@ g_ptr_array_copy (GPtrArray *array, array->len * sizeof (*array->pdata)); } + new_array->len = array->len; + return new_array; } From 625055f60e6d59ebad5c3ea8d0a0754d68eb5c1c Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 16 Jul 2019 10:16:41 +0100 Subject: [PATCH 3/7] tests: Fix array length calculations in array tests Signed-off-by: Philip Withnall --- glib/tests/array-test.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/glib/tests/array-test.c b/glib/tests/array-test.c index 6405d8003..e27670b0b 100644 --- a/glib/tests/array-test.c +++ b/glib/tests/array-test.c @@ -508,8 +508,9 @@ array_copy (gconstpointer test_data) /* Check internal parameters ('clear' flag) */ if (config->clear_) { - g_array_set_size (array_copy, array_copy->len + 5); - for (i = array_copy->len; i < array_copy->len + 5; i++) + guint old_length = array_copy->len; + g_array_set_size (array_copy, old_length + 5); + for (i = old_length; i < old_length + 5; i++) g_assert_cmpint (g_array_index (array_copy, gint, i), ==, 0); } From 121b6bc599fcbe9c5f0414a6cdbdc2d00a481ee2 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 16 Jul 2019 10:17:08 +0100 Subject: [PATCH 4/7] tests: Add missing array length checks Signed-off-by: Philip Withnall --- glib/tests/array-test.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/glib/tests/array-test.c b/glib/tests/array-test.c index e27670b0b..906812b01 100644 --- a/glib/tests/array-test.c +++ b/glib/tests/array-test.c @@ -939,6 +939,8 @@ pointer_array_copy (void) ptr_array = g_ptr_array_sized_new (0); ptr_array2 = g_ptr_array_copy (ptr_array, NULL, NULL); + g_assert_cmpuint (ptr_array2->len, ==, ptr_array->len); + g_ptr_array_unref (ptr_array); g_ptr_array_unref (ptr_array2); @@ -950,6 +952,7 @@ pointer_array_copy (void) ptr_array2 = g_ptr_array_copy (ptr_array, NULL, NULL); + g_assert_cmpuint (ptr_array2->len, ==, ptr_array->len); for (i = 0; i < array_size; i++) g_assert_cmpuint (*((gsize *) g_ptr_array_index (ptr_array2, i)), ==, i); @@ -962,6 +965,7 @@ pointer_array_copy (void) /* Test copy through GCopyFunc */ ptr_array2 = g_ptr_array_copy (ptr_array, ptr_array_copy_func, NULL); + g_assert_cmpuint (ptr_array2->len, ==, ptr_array->len); for (i = 0; i < array_size; i++) g_assert_cmpuint (*((gsize *) g_ptr_array_index (ptr_array2, i)), ==, i); From bc0fcddc1892eeda2e06ddde65379f58d0fb091d Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 16 Jul 2019 10:17:20 +0100 Subject: [PATCH 5/7] tests: Use g_ptr_array_set_free_func() rather than manual free() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We don’t need to free array elements manually. Signed-off-by: Philip Withnall --- glib/tests/array-test.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/glib/tests/array-test.c b/glib/tests/array-test.c index 906812b01..fd8ceb7e1 100644 --- a/glib/tests/array-test.c +++ b/glib/tests/array-test.c @@ -964,6 +964,7 @@ pointer_array_copy (void) /* Test copy through GCopyFunc */ ptr_array2 = g_ptr_array_copy (ptr_array, ptr_array_copy_func, NULL); + g_ptr_array_set_free_func (ptr_array2, g_free); g_assert_cmpuint (ptr_array2->len, ==, ptr_array->len); for (i = 0; i < array_size; i++) @@ -973,9 +974,6 @@ pointer_array_copy (void) g_assert_cmpuint ((gsize) g_ptr_array_index (ptr_array, i), !=, (gsize) g_ptr_array_index (ptr_array2, i)); - for (i = 0; i < array_size; i++) - free(ptr_array2->pdata[i]); - g_ptr_array_free (ptr_array2, TRUE); /* Final cleanup */ From ec61daf5033b958778814cad8e50e6a4504d161e Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 16 Jul 2019 11:35:18 +0100 Subject: [PATCH 6/7] garray: Rewrite binary search calculation to avoid integer overflow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If `right` and `left` are both near `G_MAXUINT`, it’s possible for the addition to overflow, even if the eventual result would fit in a `guint`. Avoid that by operating on the difference instead. The difference is guaranteed to be positive due to the prior `left <= right` check. Signed-off-by: Philip Withnall --- glib/garray.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/glib/garray.c b/glib/garray.c index 6e70f859e..3f13d746f 100644 --- a/glib/garray.c +++ b/glib/garray.c @@ -845,7 +845,7 @@ g_array_binary_search (GArray *array, while (left <= right) { - middle = (left + right) / 2; + middle = left + (right - left) / 2; val = compare_func (_array->data + (_array->elt_size * middle), target); if (val < 0) From c0f13f3cc813a14b270bb43b0baa85d9e6680016 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 16 Jul 2019 11:36:28 +0100 Subject: [PATCH 7/7] garray: Fix binary search for non-existent elements on the left MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If searching for an element which is smaller than every element in the array (i.e. the element being searched for is not in the array), the previous g_array_binary_search() implementation would underflow in the calculation `right = middle - 1`, and end up trying to dereference an element way off the right of the array. Fix that by checking the additions/subtractions before doing them, and bailing if the bounds are hit. We don’t need to check `middle < G_MAXUINT`, as `middle` is bounded above by `right`, which is always `<= _array->len - 1`, and `_array->len <= G_MAXUINT`. Add some tests for that, and for not-present elements in the middle of the array. Previously, the tests only checked for not-present elements which were bigger than every element in the array. Signed-off-by: Philip Withnall --- glib/garray.c | 12 ++++++---- glib/tests/array-test.c | 51 ++++++++++++++++++++++++++++++++--------- 2 files changed, 47 insertions(+), 16 deletions(-) diff --git a/glib/garray.c b/glib/garray.c index 3f13d746f..569cff892 100644 --- a/glib/garray.c +++ b/glib/garray.c @@ -848,15 +848,17 @@ g_array_binary_search (GArray *array, middle = left + (right - left) / 2; val = compare_func (_array->data + (_array->elt_size * middle), target); - if (val < 0) - left = middle + 1; - else if (val > 0) - right = middle - 1; - else + if (val == 0) { result = TRUE; break; } + else if (val < 0) + left = middle + 1; + else if (/* val > 0 && */ middle > 0) + right = middle - 1; + else + break; /* element not found */ } } diff --git a/glib/tests/array-test.c b/glib/tests/array-test.c index fd8ceb7e1..b26704e25 100644 --- a/glib/tests/array-test.c +++ b/glib/tests/array-test.c @@ -677,6 +677,9 @@ test_array_binary_search (void) g_assert_true (g_array_binary_search (garray, &i, cmpint, NULL)); + i = 0; + g_assert_false (g_array_binary_search (garray, &i, cmpint, NULL)); + i = 2; g_assert_false (g_array_binary_search (garray, &i, cmpint, NULL)); @@ -684,26 +687,32 @@ test_array_binary_search (void) /* Testing array of size 2 */ garray = g_array_sized_new (FALSE, FALSE, sizeof (guint), 2); - for (i = 0; i < 2; i++) + for (i = 1; i < 3; i++) g_array_append_val (garray, i); - for (i = 0; i < 2; i++) + for (i = 1; i < 3; i++) g_assert_true (g_array_binary_search (garray, &i, cmpint, NULL)); - i = 3; + i = 0; + g_assert_false (g_array_binary_search (garray, &i, cmpint, NULL)); + + i = 4; g_assert_false (g_array_binary_search (garray, &i, cmpint, NULL)); g_array_free (garray, TRUE); /* Testing array of size 3 */ garray = g_array_sized_new (FALSE, FALSE, sizeof (guint), 3); - for (i = 0; i < 3; i++) + for (i = 1; i < 4; i++) g_array_append_val (garray, i); - for (i = 0; i < 3; i++) + for (i = 1; i < 4; i++) g_assert_true (g_array_binary_search (garray, &i, cmpint, NULL)); - i = 4; + i = 0; + g_assert_false (g_array_binary_search (garray, &i, cmpint, NULL)); + + i = 5; g_assert_false (g_array_binary_search (garray, &i, cmpint, NULL)); g_array_free (garray, TRUE); @@ -711,23 +720,43 @@ test_array_binary_search (void) /* Testing array of size 10000 */ garray = g_array_sized_new (FALSE, FALSE, sizeof (guint), 10000); - for (i = 0; i < 10000; i++) + for (i = 1; i < 10001; i++) g_array_append_val (garray, i); - for (i = 0; i < 10000; i++) + for (i = 1; i < 10001; i++) g_assert_true (g_array_binary_search (garray, &i, cmpint, NULL)); - for (i = 0; i < 10000; i++) + for (i = 1; i < 10001; i++) { g_assert_true (g_array_binary_search (garray, &i, cmpint, &matched_index)); - g_assert_cmpint (i, ==, matched_index); + g_assert_cmpint (i, ==, matched_index + 1); } /* Testing negative result */ - i = 10001; + i = 0; g_assert_false (g_array_binary_search (garray, &i, cmpint, NULL)); g_assert_false (g_array_binary_search (garray, &i, cmpint, &matched_index)); + i = 10002; + g_assert_false (g_array_binary_search (garray, &i, cmpint, NULL)); + g_assert_false (g_array_binary_search (garray, &i, cmpint, &matched_index)); + + g_array_free (garray, TRUE); + + /* Test for a not-found element in the middle of the array. */ + garray = g_array_sized_new (FALSE, FALSE, sizeof (guint), 3); + for (i = 1; i < 10; i += 2) + g_array_append_val (garray, i); + + i = 0; + g_assert_false (g_array_binary_search (garray, &i, cmpint, NULL)); + + i = 2; + g_assert_false (g_array_binary_search (garray, &i, cmpint, NULL)); + + i = 10; + g_assert_false (g_array_binary_search (garray, &i, cmpint, NULL)); + g_array_free (garray, TRUE); }