From: Willy Tarreau <w@1wt.eu>
Date: Thu, 11 Jun 2020 14:37:35 +0000 (+0200)
Subject: BUG/MEDIUM: pattern: fix thread safety of pattern matching
X-Git-Tag: v2.2-dev10~59
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=2fc761e827071c4b976403e69836ff063994ee14;p=thirdparty%2Fhaproxy.git

BUG/MEDIUM: pattern: fix thread safety of pattern matching

Commit b5997f740 ("MAJOR: threads/map: Make acls/maps thread safe")
introduced a subtle bug in the pattern matching code. In order to cope
with the possibility that another thread might be modifying the pattern's
sample_data while it's being used, we return a thread-local static
sample_data which is a copy of the one found in the matched pattern. The
copy is performed depending on the sample_data's type. But the switch
statement misses some breaks and doesn't set the new sample_data pointer
at the right place, resulting in the original sample_data being restored
at the end before returning.

The net effect overall is that the correct sample_data is returned (hence
functionally speaking the matching works fine) but it's not thread-safe
so any del_map() or set_map() action could modify the pattern on one
thread while it's being used on another one. It doesn't seem likely to
cause a crash but could result in corrupted data appearing where the
value is consumed (e.g. when appended in a header or when logged) or an
ACL occasionally not matching after a map lookup.

This fix should be backported as far as 1.8.

Thanks to Tim for reporting it and to Emeric for the analysis.
---

diff --git a/src/pattern.c b/src/pattern.c
index 274e773ec4..bd8b25a618 100644
--- a/src/pattern.c
+++ b/src/pattern.c
@@ -2557,17 +2557,22 @@ struct pattern *pattern_exec_match(struct pattern_head *head, struct sample *smp
 						if (static_sample_data.u.str.data >= static_sample_data.u.str.size)
 							static_sample_data.u.str.data = static_sample_data.u.str.size - 1;
 						memcpy(static_sample_data.u.str.area,
-						       pat->data->u.str.area,
-						       static_sample_data.u.str.data);
+						       pat->data->u.str.area, static_sample_data.u.str.data);
 						static_sample_data.u.str.area[static_sample_data.u.str.data] = 0;
+						pat->data = &static_sample_data;
+						break;
+
 					case SMP_T_IPV4:
 					case SMP_T_IPV6:
 					case SMP_T_SINT:
 						memcpy(&static_sample_data, pat->data, sizeof(struct sample_data));
+						pat->data = &static_sample_data;
+						break;
 					default:
+						/* unimplemented pattern type */
 						pat->data = NULL;
+						break;
 				}
-				pat->data = &static_sample_data;
 			}
 			HA_RWLOCK_RDUNLOCK(PATEXP_LOCK, &list->expr->lock);
 			return pat;