From: Date: November 4 2008 1:18pm
Subject: bzr commit into mysql-5.1 branch (kristofer.pettersson:2686) Bug#38883
List-Archive: http://lists.mysql.com/commits/57779
X-Bug: 38883
Message-Id: <0K9T002XM66GOX20@fe-emea-09.sun.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7BIT

#At file:///home/thek/Development/cpp/mysqlbzr/mysql-5.1-bug38883/

 2686 Kristofer Pettersson	2008-11-04
      Bug#38883 thd_security_context is not thread safe, crashes?
            
      Innodb monitor could cause a server crash because of invalid access to a
      shared variable in a concurrent environment.
                  
      This patch adds a guard to protect against crashes but not against
      inconsistent values because of performance reasons.
            
      This patch is not complete without additional changes to InnoDB where all
      access to thd_security_context have a preceding call to
      innobase_mysql_prepare_print_arbitrary_thd
modified:
  sql/sql_class.cc

per-file messages:
  sql/sql_class.cc
    * Enforce requriement of lock protection in thd_security_context to protect
    thd->query.
    * Attempt snapshot of static memory pointer proc_info to avoid null pointers.
=== modified file 'sql/sql_class.cc'
--- a/sql/sql_class.cc	2008-10-24 13:58:48 +0000
+++ b/sql/sql_class.cc	2008-11-04 12:26:35 +0000
@@ -306,28 +306,43 @@ void thd_inc_row_count(THD *thd)
   thd->row_count++;
 }
 
-/*
+
+/**
   Dumps a text description of a thread, its security context
   (user, host) and the current query.
 
-  SYNOPSIS
-    thd_security_context()
-    thd                 current thread context
-    buffer              pointer to preferred result buffer
-    length              length of buffer
-    max_query_len       how many chars of query to copy (0 for all)
-
-  RETURN VALUES
-    pointer to string
+  @param thd current thread context
+  @param buffer pointer to preferred result buffer
+  @param length length of buffer
+  @param max_query_len how many chars of query to copy (0 for all)
+
+  @req LOCK_thread_count
+ 
+  @return Pointer to string
 */
+
 extern "C"
 char *thd_security_context(THD *thd, char *buffer, unsigned int length,
                            unsigned int max_query_len)
 {
+#ifndef DBUG_OFF
+  if (current_thd != thd)
+    safe_mutex_assert_owner(&LOCK_thread_count);
+#endif
   String str(buffer, length, &my_charset_latin1);
   const Security_context *sctx= &thd->main_security_ctx;
   char header[64];
   int len;
+  /*
+    The pointers thd->query and thd->proc_info might change since they are
+    being modified concurrently. This is acceptable for proc_info since its
+    values doesn't have to very accurate and the memory it points to is static,
+    but we need to attempt a snapshot on the pointer values to avoid using NULL
+    values. The pointer to thd->query however, doesn't point to static memory
+    and has to be protected by LOCK_thread_count or risk pointing to
+    uninitialized memory.
+  */
+  const char *proc_info= thd->proc_info;
 
   len= my_snprintf(header, sizeof(header),
                    "MySQL thread id %lu, query id %lu",
@@ -353,10 +368,10 @@ char *thd_security_context(THD *thd, cha
     str.append(sctx->user);
   }
 
-  if (thd->proc_info)
+  if (proc_info)
   {
     str.append(' ');
-    str.append(thd->proc_info);
+    str.append(proc_info);
   }
 
   if (thd->query)