From: Willy Tarreau <w@1wt.eu>
Date: Tue, 5 May 2020 14:13:36 +0000 (+0200)
Subject: BUG/MINOR: threads: fix multiple use of argument inside HA_ATOMIC_UPDATE_{MIN,MAX}()
X-Git-Tag: v2.2-dev7~3
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=a4d9ee3d1cc7b321be420742263b67620036bb71;p=thirdparty%2Fhaproxy.git

BUG/MINOR: threads: fix multiple use of argument inside HA_ATOMIC_UPDATE_{MIN,MAX}()

Just like in previous patch, it happens that HA_ATOMIC_UPDATE_MIN() and
HA_ATOMIC_UPDATE_MAX() would evaluate the (val) argument up to 3 times.
However this time it affects both thread and non-thread versions. It's
strange because the copy was properly performed for the (new) argument
in order to avoid this. Anyway it was done for the "val" one as well.

A quick code inspection showed that this currently has no effect as
these macros are fairly limited in usage.

It would be best to backport this for long-term stability (till 1.8)
but it will not fix an existing bug.
---

diff --git a/include/common/hathreads.h b/include/common/hathreads.h
index 1350f19046..cc54574fad 100644
--- a/include/common/hathreads.h
+++ b/include/common/hathreads.h
@@ -135,20 +135,22 @@ extern THREAD_LOCAL struct thread_info *ti; /* thread_info for the current threa
 #define HA_ATOMIC_STORE(val, new)    ({*(val) = new;})
 #define HA_ATOMIC_UPDATE_MAX(val, new)					\
 	({								\
+		typeof(val) __val = (val);                              \
 		typeof(*(val)) __new_max = (new);			\
 									\
-		if (*(val) < __new_max)					\
-			*(val) = __new_max;				\
-		*(val);							\
+		if (*__val < __new_max)					\
+			*__val = __new_max;				\
+		*__val;							\
 	})
 
 #define HA_ATOMIC_UPDATE_MIN(val, new)					\
 	({								\
+		typeof(val) __val = (val);                              \
 		typeof(*(val)) __new_min = (new);			\
 									\
-		if (*(val) > __new_min)					\
-			*(val) = __new_min;				\
-		*(val);							\
+		if (*__val > __new_min)					\
+			*__val = __new_min;				\
+		*__val;							\
 	})
 
 #define HA_BARRIER() do { } while (0)
@@ -410,21 +412,23 @@ static inline unsigned long thread_isolated()
 
 #define HA_ATOMIC_UPDATE_MAX(val, new)					\
 	({								\
-		typeof(*(val)) __old_max = *(val);			\
+		typeof(val) __val = (val);                              \
+		typeof(*(val)) __old_max = *__val;			\
 		typeof(*(val)) __new_max = (new);			\
 									\
 		while (__old_max < __new_max &&				\
-		       !HA_ATOMIC_CAS(val, &__old_max, __new_max));	\
-		*(val);							\
+		       !HA_ATOMIC_CAS(__val, &__old_max, __new_max));	\
+		*__val;							\
 	})
 #define HA_ATOMIC_UPDATE_MIN(val, new)					\
 	({								\
-		typeof(*(val)) __old_min = *(val);			\
+		typeof(val) __val = (val);                              \
+		typeof(*(val)) __old_min = *__val;			\
 		typeof(*(val)) __new_min = (new);			\
 									\
 		while (__old_min > __new_min &&				\
-		       !HA_ATOMIC_CAS(val, &__old_min, __new_min));	\
-		*(val);							\
+		       !HA_ATOMIC_CAS(__val, &__old_min, __new_min));	\
+		*__val;							\
 	})
 
 #define HA_BARRIER() pl_barrier()