From: Yann Ylavic <ylavic@apache.org>
Date: Tue, 9 Mar 2021 16:35:03 +0000 (+0000)
Subject: Merge r1887244, r1887245 from trunk:
X-Git-Tag: 2.4.47~65
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=5ae750cc05eb182da6e30abcbcb9ef84a7248b07;p=thirdparty%2Fapache%2Fhttpd.git

Merge r1887244, r1887245 from trunk:

Fix a potential duplicated ID generation issue under heavy load.
This is due to a non thread safe use of a counter.

Use a counter for each thread instead to avoid the issue.

PR 65159


Follow-up to r1887244.

Wrong version of the patch attached :(


Submitted by: jailletc36
Reviewed by: jailletc36, ylavic, jorton


git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x@1887386 13f79535-47bb-0310-9956-ffa450edef68
---

diff --git a/CHANGES b/CHANGES
index fa3d811973a..5b7ea19b8f1 100644
--- a/CHANGES
+++ b/CHANGES
@@ -1,6 +1,10 @@
                                                          -*- coding: utf-8 -*-
 Changes with Apache 2.4.47
 
+  *) mod_unique_id: Fix potential duplicated ID generation under heavy load.
+     PR 65159
+     [Jonas Müntener <jonas.muentener ergon.ch>, Christophe Jaillet]
+
   *) "[mod_dav_fs etag handling] should really honor the FileETag setting".
      - It now does.
      - Add "Digest" to FileETag directive, allowing a strong ETag to be
diff --git a/modules/metadata/mod_unique_id.c b/modules/metadata/mod_unique_id.c
index 4a92beaefb8..3327f4c62f5 100644
--- a/modules/metadata/mod_unique_id.c
+++ b/modules/metadata/mod_unique_id.c
@@ -42,12 +42,6 @@ typedef struct {
 
 /* We are using thread_index (the index into the scoreboard), because we
  * cannot guarantee the thread_id will be an integer.
- *
- * This code looks like it won't give a unique ID with the new thread logic.
- * It will.  The reason is, we don't increment the counter in a thread_safe
- * manner.  Because the thread_index is also in the unique ID now, this does
- * not matter.  In order for the id to not be unique, the same thread would
- * have to get the same counter twice in the same second.
  */
 
 /* Comments:
@@ -69,7 +63,7 @@ typedef struct {
  * The stamp and counter are used to distinguish all hits for a
  * particular root.  The stamp is updated using r->request_time,
  * saving cpu cycles.  The counter is never reset, and is used to
- * permit up to 64k requests in a single second by a single child.
+ * permit up to 64k requests in a single second by a single thread.
  *
  * The 144-bits of unique_id_rec are encoded using the alphabet
  * [A-Za-z0-9@-], resulting in 24 bytes of printable characters.  That is then
@@ -116,12 +110,6 @@ typedef struct {
  * htonl/ntohl. Well, this shouldn't be a problem till year 2106.
  */
 
-/*
- * XXX: We should have a per-thread counter and not use cur_unique_id.counter
- * XXX: in all threads, because this is bad for performance on multi-processor
- * XXX: systems: Writing to the same address from several CPUs causes cache
- * XXX: thrashing.
- */
 static unique_id_rec cur_unique_id;
 
 /*
@@ -182,11 +170,13 @@ static const char uuencoder[64] = {
     '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '-', '_',
 };
 
+#define THREADED_COUNTER "unique_id_counter"
+
 static const char *gen_unique_id(const request_rec *r)
 {
     char *str;
     /*
-     * Buffer padded with two final bytes, used to copy the unique_id_red
+     * Buffer padded with two final bytes, used to copy the unique_id_rec
      * structure without the internal paddings that it could have.
      */
     unique_id_rec new_unique_id;
@@ -198,8 +188,35 @@ static const char *gen_unique_id(const request_rec *r)
     unsigned short counter;
     int i,j,k;
 
+#if APR_HAS_THREADS
+    apr_status_t rv;
+    unsigned short *pcounter;
+    apr_thread_t *thread = r->connection->current_thread;
+    
+    rv = apr_thread_data_get((void **)&pcounter, THREADED_COUNTER, thread);
+    if (rv == APR_SUCCESS && pcounter) {
+        counter = *pcounter;
+    }
+    else
+#endif
+    {
+        counter = cur_unique_id.counter;
+    }
+
     memcpy(&new_unique_id.root, &cur_unique_id.root, ROOT_SIZE);
-    new_unique_id.counter = cur_unique_id.counter;
+    new_unique_id.counter = htons(counter++);
+#if APR_HAS_THREADS
+    if (!pcounter) {
+        pcounter = apr_palloc(apr_thread_pool_get(thread), sizeof(*pcounter));
+    }
+    
+    *pcounter = counter;
+    rv = apr_thread_data_set(pcounter, THREADED_COUNTER, NULL, thread);
+    if (rv != APR_SUCCESS)
+#endif
+    {
+        cur_unique_id.counter = counter;
+    }
     new_unique_id.stamp = htonl((unsigned int)apr_time_sec(r->request_time));
     new_unique_id.thread_index = htonl((unsigned int)r->connection->id);
 
@@ -233,11 +250,6 @@ static const char *gen_unique_id(const request_rec *r)
     }
     str[k++] = '\0';
 
-    /* and increment the identifier for the next call */
-
-    counter = ntohs(new_unique_id.counter) + 1;
-    cur_unique_id.counter = htons(counter);
-
     return str;
 }