From 85c267afa7ce4697a1231649de815b2556b3950f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 9 Dec 2019 18:24:41 +0100 Subject: [PATCH 1/4] macro: avoid subtraction overflow in ALIGN_POWER2() --- src/basic/macro.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/basic/macro.h b/src/basic/macro.h index 18e5085669c..712bb422b1c 100644 --- a/src/basic/macro.h +++ b/src/basic/macro.h @@ -163,6 +163,11 @@ static inline size_t ALIGN_TO(size_t l, size_t ali) { /* align to next higher power-of-2 (except for: 0 => 0, overflow => 0) */ static inline unsigned long ALIGN_POWER2(unsigned long u) { + + /* Avoid subtraction overflow */ + if (u == 0) + return 0; + /* clz(0) is undefined */ if (u == 1) return 1; From e49e4c33dc965b09d8d4cb95780f79de6d1d5eb5 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 9 Dec 2019 18:26:10 +0100 Subject: [PATCH 2/4] macro: introduce new GREEDY_ALLOC_ROUND_UP() helper --- src/basic/macro.h | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/basic/macro.h b/src/basic/macro.h index 712bb422b1c..5aa7f59c0ba 100644 --- a/src/basic/macro.h +++ b/src/basic/macro.h @@ -179,6 +179,29 @@ static inline unsigned long ALIGN_POWER2(unsigned long u) { return 1UL << (sizeof(u) * 8 - __builtin_clzl(u - 1UL)); } +static inline size_t GREEDY_ALLOC_ROUND_UP(size_t l) { + size_t m; + + /* Round up allocation sizes a bit to some reasonable, likely larger value. This is supposed to be + * used for cases which are likely called in an allocation loop of some form, i.e. that repetitively + * grow stuff, for example strv_extend() and suchlike. + * + * Note the difference to GREEDY_REALLOC() here, as this helper operates on a single size value only, + * and rounds up to next multiple of 2, needing no further counter. + * + * Note the benefits of direct ALIGN_POWER2() usage: type-safety for size_t, sane handling for very + * small (i.e. <= 2) and safe handling for very large (i.e. > SSIZE_MAX) values. */ + + if (l <= 2) + return 2; /* Never allocate less than 2 of something. */ + + m = ALIGN_POWER2(l); + if (m == 0) /* overflow? */ + return l; + + return m; +} + #ifndef __COVERITY__ # define VOID_0 ((void)0) #else From 47ac31f792a8856f29ea4ac38d6b366e88ac7d34 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 9 Dec 2019 18:29:29 +0100 Subject: [PATCH 3/4] test-util: add more tests for ALIGN_POWER2 --- src/test/test-util.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/test/test-util.c b/src/test/test-util.c index 61725bdf087..76dd72a5980 100644 --- a/src/test/test-util.c +++ b/src/test/test-util.c @@ -26,7 +26,19 @@ static void test_align_power2(void) { assert_se(ALIGN_POWER2(1) == 1); assert_se(ALIGN_POWER2(2) == 2); assert_se(ALIGN_POWER2(3) == 4); + assert_se(ALIGN_POWER2(4) == 4); + assert_se(ALIGN_POWER2(5) == 8); + assert_se(ALIGN_POWER2(6) == 8); + assert_se(ALIGN_POWER2(7) == 8); + assert_se(ALIGN_POWER2(9) == 16); + assert_se(ALIGN_POWER2(10) == 16); + assert_se(ALIGN_POWER2(11) == 16); assert_se(ALIGN_POWER2(12) == 16); + assert_se(ALIGN_POWER2(13) == 16); + assert_se(ALIGN_POWER2(14) == 16); + assert_se(ALIGN_POWER2(15) == 16); + assert_se(ALIGN_POWER2(16) == 16); + assert_se(ALIGN_POWER2(17) == 32); assert_se(ALIGN_POWER2(ULONG_MAX) == 0); assert_se(ALIGN_POWER2(ULONG_MAX - 1) == 0); From 6047637645ac8038edbca12bf91d030eddd59972 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 9 Dec 2019 18:30:00 +0100 Subject: [PATCH 4/4] strv: when growing strv arrays piecemeal actually allocate memory in exponential steps Let's improve memory allocation for call such as strv_extend() that just one item to an strv: these are often called in a loop, where they used to be very ineffecient, since we'd allocate byte-exact space. With this change let's improve on that, by allocating exponentially by rounding up to the next exponent of 2. This way we get GREEDY_REALLOC()-like behaviour without passing around state. In fact this should be good enough so that we could replace existing loops around GREEDY_REALLOC() for strv build-up with plain strv_extend() and get similar behaviour. --- src/basic/strv.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/src/basic/strv.c b/src/basic/strv.c index 30fab630745..92e528940a3 100644 --- a/src/basic/strv.c +++ b/src/basic/strv.c @@ -193,7 +193,10 @@ int strv_extend_strv(char ***a, char **b, bool filter_duplicates) { p = strv_length(*a); q = strv_length(b); - t = reallocarray(*a, p + q + 1, sizeof(char *)); + if (p >= SIZE_MAX - q) + return -ENOMEM; + + t = reallocarray(*a, GREEDY_ALLOC_ROUND_UP(p + q + 1), sizeof(char *)); if (!t) return -ENOMEM; @@ -383,19 +386,18 @@ char *strv_join_prefix(char **l, const char *separator, const char *prefix) { int strv_push(char ***l, char *value) { char **c; - size_t n, m; + size_t n; if (!value) return 0; n = strv_length(*l); - /* Increase and check for overflow */ - m = n + 2; - if (m < n) + /* Check for overflow */ + if (n > SIZE_MAX-2) return -ENOMEM; - c = reallocarray(*l, m, sizeof(char*)); + c = reallocarray(*l, GREEDY_ALLOC_ROUND_UP(n + 2), sizeof(char*)); if (!c) return -ENOMEM; @@ -408,19 +410,19 @@ int strv_push(char ***l, char *value) { int strv_push_pair(char ***l, char *a, char *b) { char **c; - size_t n, m; + size_t n; if (!a && !b) return 0; n = strv_length(*l); - /* increase and check for overflow */ - m = n + !!a + !!b + 1; - if (m < n) + /* Check for overflow */ + if (n > SIZE_MAX-3) return -ENOMEM; - c = reallocarray(*l, m, sizeof(char*)); + /* increase and check for overflow */ + c = reallocarray(*l, GREEDY_ALLOC_ROUND_UP(n + !!a + !!b + 1), sizeof(char*)); if (!c) return -ENOMEM; @@ -846,8 +848,10 @@ int strv_extend_n(char ***l, const char *value, size_t n) { /* Adds the value n times to l */ k = strv_length(*l); + if (n >= SIZE_MAX - k) + return -ENOMEM; - nl = reallocarray(*l, k + n + 1, sizeof(char *)); + nl = reallocarray(*l, GREEDY_ALLOC_ROUND_UP(k + n + 1), sizeof(char *)); if (!nl) return -ENOMEM;