From: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
Date: Mon, 21 Mar 2022 10:03:00 +0000 (+0100)
Subject: basic/strv: avoid potential UB with references to array[-1]
X-Git-Tag: v251-rc1~88
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=9b01798b98d1d8e7cecb2eaf49aa6cc39d57ae0d;p=thirdparty%2Fsystemd.git

basic/strv: avoid potential UB with references to array[-1]

"""
Given an array a[N] of N elements of type T:
- Forming a pointer &a[i] (or a + i) with 0 ≤ i ≤ N is safe.
- Forming a pointer &a[i] with i < 0 or i > N causes undefined behavior.
- Dereferencing a pointer &a[i] with 0 ≤ i < N is safe.
- Dereferencing a pointer &a[i] with i < 0 or i ≥ N causes undefined behavior.
"""

As pointed by by @medhefgo, here we were forming a pointer to a[-1]. a itself
wasn't NULL, so a > 0, and a-1 was also >= 0, and this didn't seem to cause any
problems. But it's better to be formally correct, especially if we move the
code to src/fundamental/ later on and compile it differently.

Compilation shows no size change (with -O0 -g) on build/systemd, so this should
have no effect whatsoever.
---

diff --git a/src/basic/strv.h b/src/basic/strv.h
index 985499272f9..bc76a2861cb 100644
--- a/src/basic/strv.h
+++ b/src/basic/strv.h
@@ -133,8 +133,8 @@ bool strv_overlap(char * const *a, char * const *b) _pure_;
                                 size_t _len = strv_length(h);           \
                                 _len > 0 ? h + _len - 1 : NULL;         \
                         });                                             \
-             i && (s = i) >= h;                                         \
-             i--)
+             (s = i);                                                   \
+             i > h ? i-- : (i = NULL))
 
 #define STRV_FOREACH_BACKWARDS(s, l)                                    \
         _STRV_FOREACH_BACKWARDS(s, l, UNIQ_T(h, UNIQ), UNIQ_T(i, UNIQ))
diff --git a/src/test/test-strv.c b/src/test/test-strv.c
index ae3e2798c2b..edb782eb0d7 100644
--- a/src/test/test-strv.c
+++ b/src/test/test-strv.c
@@ -639,8 +639,13 @@ TEST(strv_foreach_backwards) {
         STRV_FOREACH_BACKWARDS(check, (char**) NULL)
                 assert_not_reached();
 
-        STRV_FOREACH_BACKWARDS(check, (char**) { NULL })
+        STRV_FOREACH_BACKWARDS(check, STRV_MAKE_EMPTY)
                 assert_not_reached();
+
+        unsigned count = 0;
+        STRV_FOREACH_BACKWARDS(check, STRV_MAKE("ONE"))
+                count++;
+        assert_se(count == 1);
 }
 
 TEST(strv_foreach_pair) {