From: Yann Ylavic <ylavic@apache.org>
Date: Mon, 6 Sep 2021 10:17:04 +0000 (+0000)
Subject: Merge r1892914 from trunk:
X-Git-Tag: candidate-2.4.49-rc1~3^2~39
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=123e27c560044835c81873d86111ccdb7dde5407;p=thirdparty%2Fapache%2Fhttpd.git

Merge r1892914 from trunk:

Revert r1887244 and r1887245 which causes issues on Windows

Submitted by: jailletc36
Reviewed by: jailletc36, icing, jorton


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

diff --git a/changes-entries/revert_r887244_and_r887245.txt b/changes-entries/revert_r887244_and_r887245.txt
new file mode 100644
index 00000000000..4b65e1d9f92
--- /dev/null
+++ b/changes-entries/revert_r887244_and_r887245.txt
@@ -0,0 +1,4 @@
+  *) Revert "mod_unique_id: Fix potential duplicated ID generation under heavy load.
+     PR 65159" added in 2.4.47.
+     This causes issue on Windows.
+     [Christophe Jaillet]
diff --git a/modules/metadata/mod_unique_id.c b/modules/metadata/mod_unique_id.c
index 3327f4c62f5..4a92beaefb8 100644
--- a/modules/metadata/mod_unique_id.c
+++ b/modules/metadata/mod_unique_id.c
@@ -42,6 +42,12 @@ 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:
@@ -63,7 +69,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 thread.
+ * permit up to 64k requests in a single second by a single child.
  *
  * 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
@@ -110,6 +116,12 @@ 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;
 
 /*
@@ -170,13 +182,11 @@ 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_rec
+     * Buffer padded with two final bytes, used to copy the unique_id_red
      * structure without the internal paddings that it could have.
      */
     unique_id_rec new_unique_id;
@@ -188,35 +198,8 @@ 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 = 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.counter = cur_unique_id.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);
 
@@ -250,6 +233,11 @@ 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;
 }