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

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1887244 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/changes-entries/mod_unique_id.txt b/changes-entries/mod_unique_id.txt
new file mode 100644 (file)
index 0000000..e5d8dba
--- /dev/null
+++ b/changes-entries/mod_unique_id.txt
@@ -0,0 +1,3 @@
+  *) mod_unique_id: Fix potential duplicated ID generation under heavy load.
+     PR 65159
+     [Jonas Müntener <jonas.muentener ergon.ch>, Christophe Jaillet]

diff --git a/modules/metadata/mod_unique_id.c b/modules/metadata/mod_unique_id.c
index 4a92beaefb8e3cd90706989460019762381af031..399adf8713e0996d949e419f700e74a07af76104 100644 (file)
--- 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_thread_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;
@@ -197,9 +187,36 @@ static const char *gen_unique_id(const request_rec *r)
     unsigned char *x,*y;
     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(&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(r->connection->current_thread),
+                              sizeof(*pcounter));
+    }
+    
+    *pcounter = counter;
+    rv = apr_thread_data_set(pcounter, THREADED_COUNTER, 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;
 }