BUG/MEDIUM: stick-tables: fix double-decrement of tracked entries

Mailing list participant "mlist" reported negative conn_cur values in
stick tables as the result of "tcp-request connection track-sc". The
reason is that after the stick entry it copied from the session to the
stream, both the session and the stream grab a reference to the entry
and when the stream ends, it decrements one reference and one connection,
then the same is done for the session.

In fact this problem was already encountered slightly differently in the
past and addressed by Thierry using the patch below as it was believed by
then to be only a refcount issue since it was the observable symptom :

   827752e "BUG/MEDIUM: stick-tables: refcount error after copying SC..."

In reality the problem is that the stream must touch neither the refcount
nor the connection count for entries it inherits from the session. While
we have no way to tell whether a track entry was inherited from the session
(since they're simply memcpy'd), it is possible to prevent the stream from
touching an entry that already exists in the session because that's a
guarantee that it was inherited from it.

Note that it may be a temporary fix. Maybe in the future when a session
gives birth to multiple streams we'll face a situation where a session may
be updated to add more tracked entries after instanciating some streams.
The correct long-term fix is to mark some tracked entries as shared or
private (or RO/RW). That will allow the session to track more entries
even after the same trackers are being used by early streams.

No backport is needed, this is only caused by the session/stream split in 1.6.

include/proto/stream.h

@@ -88,7 +88,8 @@ static inline enum obj_type *strm_orig(const struct stream *strm)
 
 /* Remove the refcount from the stream to the tracked counters, and clear the
  * pointer to ensure this is only performed once. The caller is responsible for
- * ensuring that the pointer is valid first.
+ * ensuring that the pointer is valid first. We must be extremely careful not
+ * to touch the entries we inherited from the session.
  */
 static inline void stream_store_counters(struct stream *s)
 {
@@ -98,6 +99,10 @@ static inline void stream_store_counters(struct stream *s)
 	for (i = 0; i < MAX_SESS_STKCTR; i++) {
 		if (!stkctr_entry(&s->stkctr[i]))
 			continue;
+
+		if (stkctr_entry(&s->sess->stkctr[i]))
+			continue;
+
 		ptr = stktable_data_ptr(s->stkctr[i].table, stkctr_entry(&s->stkctr[i]), STKTABLE_DT_CONN_CUR);
 		if (ptr)
 			stktable_data_cast(ptr, conn_cur)--;
@@ -109,7 +114,8 @@ static inline void stream_store_counters(struct stream *s)
 
 /* Remove the refcount from the stream counters tracked at the content level if
  * any, and clear the pointer to ensure this is only performed once. The caller
- * is responsible for ensuring that the pointer is valid first.
+ * is responsible for ensuring that the pointer is valid first. We must be
+ * extremely careful not to touch the entries we inherited from the session.
  */
 static inline void stream_stop_content_counters(struct stream *s)
 {
@@ -120,6 +126,9 @@ static inline void stream_stop_content_counters(struct stream *s)
 		if (!stkctr_entry(&s->stkctr[i]))
 			continue;
 
+		if (stkctr_entry(&s->sess->stkctr[i]))
+			continue;
+
 		if (!(stkctr_flags(&s->stkctr[i]) & STKCTR_TRACK_CONTENT))
 			continue;
 

src/stream.c

@@ -71,7 +71,6 @@ struct stream *stream_new(struct session *sess, struct task *t, enum obj_type *o
 	struct stream *s;
 	struct connection *conn = objt_conn(origin);
 	struct appctx *appctx   = objt_appctx(origin);
-	int i;
 
 	if (unlikely((s = pool_alloc2(pool2_stream)) == NULL))
 		return s;
@@ -108,13 +107,13 @@ struct stream *stream_new(struct session *sess, struct task *t, enum obj_type *o
 	s->current_rule_list = NULL;
 	s->current_rule = NULL;
 
-	/* Copy SC counters for the stream. Each SC counter will be used by
+	/* Copy SC counters for the stream. We don't touch refcounts because
-	 * the stream, so we need to increment the refcount.
+	 * any reference we have is inherited from the session. Since the stream
+	 * doesn't exist without the session, the session's existence guarantees
+	 * we don't lose the entry. During the store operation, the stream won't
+	 * touch these ones.
 	 */
 	memcpy(s->stkctr, sess->stkctr, sizeof(s->stkctr));
-	for (i = 0; i < MAX_SESS_STKCTR; i++)
-		if (stkctr_entry(&s->stkctr[i]))
-			stkctr_entry(&s->stkctr[i])->ref_cnt++;
 
 	s->sess = sess;
 	s->si[0].flags = SI_FL_NONE;