Fix for Bug#3796 "Prepared statement, select concat(<parameter>,<column>), wrong

result": new Item_param member for use in val_str()


sql/item.cc:
  Fix for Bug#3796:
  - return str_value_ptr from val_str to prevent modifications of parameter 
  value.
sql/item.h:
  Fix for bug#3796:
  - Item::val_str() method semantics documented
  - new member of Item_param
tests/client_test.c:
  Test case for bug #3796 added. A few compile-time warnings removed.

sql/item.cc

@@ -753,9 +753,10 @@ void Item_param::reset()
     str_value.free();
   else
     str_value.length(0);
+  str_value_ptr.length(0);
   /*
-    We must prevent all charset conversions unless data of str_value
-    has been written to the binary log.
+    We must prevent all charset conversions untill data has been written
+    to the binary log.
   */
   str_value.set_charset(&my_charset_bin);
   state= NO_VALUE;
@@ -866,7 +867,7 @@ String *Item_param::val_str(String* str)
   switch (state) {
   case STRING_VALUE:
   case LONG_DATA_VALUE:
-    return &str_value;
+    return &str_value_ptr;
   case REAL_VALUE:
     str->set(value.real, NOT_FIXED_DEC, &my_charset_bin);
     return str;
@@ -980,6 +981,12 @@ bool Item_param::convert_str_value(THD *thd)
     }
     max_length= str_value.length();
     decimals= 0;
+    /*
+      str_value_ptr is returned from val_str(). It must be not alloced
+      to prevent it's modification by val_str() invoker.
+    */
+    str_value_ptr.set(str_value.ptr(), str_value.length(),
+                      str_value.charset());
   }
   return rc;
 }

sql/item.h

@@ -160,6 +160,31 @@ public:
   /* valXXX methods must return NULL or 0 or 0.0 if null_value is set. */
   virtual double val()=0;
   virtual longlong val_int()=0;
+  /*
+    Return string representation of this item object.
+
+    The argument to val_str() is an allocated buffer this or any
+    nested Item object can use to store return value of this method.
+    This buffer should only be used if the item itself doesn't have an
+    own String buffer. In case when the item maintains it's own string
+    buffer, it's preferrable to return it instead to minimize number of
+    mallocs/memcpys.
+    The caller of this method can modify returned string, but only in
+    case when it was allocated on heap, (is_alloced() is true).  This
+    allows the caller to efficiently use a buffer allocated by a child
+    without having to allocate a buffer of it's own. The buffer, given
+    to val_str() as agrument, belongs to the caller and is later used
+    by the caller at it's own choosing.
+    A few implications from the above:
+    - unless you return a string object which only points to your buffer
+      but doesn't manages it you should be ready that it will be
+      modified.
+    - even for not allocated strings (is_alloced() == false) the caller
+      can change charset (see Item_func_{typecast/binary}. XXX: is this
+      a bug?
+    - still you should try to minimize data copying and return internal
+      object whenever possible.
+  */
   virtual String *val_str(String*)=0;
   virtual Field *get_tmp_table_field() { return 0; }
   virtual Field *tmp_table_field(TABLE *t_arg) { return 0; }
@@ -390,6 +415,9 @@ public:
   void print(String *str) { str->append("NULL", 4); }
 };
 
+
+/* Item represents one placeholder ('?') of prepared statement */
+
 class Item_param :public Item
 {
 public:    
@@ -399,6 +427,17 @@ public:
     STRING_VALUE, TIME_VALUE, LONG_DATA_VALUE
   } state;
 
+  /*
+    A buffer for string and long data values. Historically all allocated
+    values returned from val_str() were treated as eligible to
+    modification. I. e. in some cases Item_func_concat can append it's
+    second argument to return value of the first one. Because of that we
+    can't return the original buffer holding string data from val_str(),
+    and have to have one buffer for data and another just pointing to
+    the data. This is the latter one and it's returned from val_str().
+    Can not be declared inside the union as it's not a POD type.
+  */
+  String str_value_ptr;
   union
   {
     longlong integer;                           

tests/client_test.c

@@ -9670,12 +9670,12 @@ static void test_union_param()
 
   /* bind parameters */
   bind[0].buffer_type=    FIELD_TYPE_STRING;
-  bind[0].buffer=         &my_val;
+  bind[0].buffer=         (char*) &my_val;
   bind[0].buffer_length=  4;
   bind[0].length=         &my_length;
   bind[0].is_null=        (char*)&my_null;
   bind[1].buffer_type=    FIELD_TYPE_STRING;
-  bind[1].buffer=         &my_val;
+  bind[1].buffer=         (char*) &my_val;
   bind[1].buffer_length=  4;
   bind[1].length=         &my_length;
   bind[1].is_null=        (char*)&my_null;
@@ -9872,7 +9872,90 @@ static void test_ps_i18n()
   mysql_stmt_close(stmt);
 
   stmt_text= "DROP TABLE t1";
-  mysql_real_query(mysql, stmt_text, strlen(stmt_text));
+  rc= mysql_real_query(mysql, stmt_text, strlen(stmt_text));
+  myquery(rc);
+  stmt_text= "SET NAMES DEFAULT";
+  rc= mysql_real_query(mysql, stmt_text, strlen(stmt_text));
+  myquery(rc);
+}
+
+
+static void test_bug3796()
+{
+  MYSQL_STMT *stmt;
+  MYSQL_BIND bind[1];
+  const char *concat_arg0= "concat_with_";
+  const int OUT_BUFF_SIZE= 30;
+  char out_buff[OUT_BUFF_SIZE];
+  char canonical_buff[OUT_BUFF_SIZE];
+  ulong out_length;
+  const char *stmt_text;
+  int rc;
+  
+  myheader("test_bug3796");
+
+  /* Create and fill test table */
+  stmt_text= "DROP TABLE IF EXISTS t1";
+  rc= mysql_real_query(mysql, stmt_text, strlen(stmt_text));
+  myquery(rc);
+
+  stmt_text= "CREATE TABLE t1 (a INT, b VARCHAR(30))";
+  rc= mysql_real_query(mysql, stmt_text, strlen(stmt_text));
+  myquery(rc);
+  
+  stmt_text= "INSERT INTO t1 VALUES(1,'ONE'), (2,'TWO')";
+  rc= mysql_real_query(mysql, stmt_text, strlen(stmt_text));
+  myquery(rc);
+
+  /* Create statement handle and prepare it with select */
+  stmt = mysql_stmt_init(mysql);
+  stmt_text= "SELECT concat(?, b) FROM t1";
+
+  rc= mysql_stmt_prepare(stmt, stmt_text, strlen(stmt_text));
+  check_execute(stmt, rc);
+  
+  /* Bind input buffers */
+  bzero(bind, sizeof(bind));
+
+  bind[0].buffer_type= MYSQL_TYPE_STRING;
+  bind[0].buffer= (char*) concat_arg0;
+  bind[0].buffer_length= strlen(concat_arg0);
+ 
+  mysql_stmt_bind_param(stmt, bind);
+
+  /* Execute the select statement */
+  rc= mysql_stmt_execute(stmt);
+  check_execute(stmt, rc);
+
+  bind[0].buffer= (char*) out_buff;
+  bind[0].buffer_length= OUT_BUFF_SIZE;
+  bind[0].length= &out_length;
+
+  mysql_stmt_bind_result(stmt, bind);
+
+  rc= mysql_stmt_fetch(stmt);
+  printf("Concat result: '%s'\n", out_buff);
+  check_execute(stmt, rc);
+  strcpy(canonical_buff, concat_arg0);
+  strcat(canonical_buff, "ONE");
+  assert(strlen(canonical_buff) == out_length &&
+         strncmp(out_buff, canonical_buff, out_length) == 0);
+
+  rc= mysql_stmt_fetch(stmt);
+  check_execute(stmt, rc);
+  strcpy(canonical_buff + strlen(concat_arg0), "TWO");
+  assert(strlen(canonical_buff) == out_length &&
+         strncmp(out_buff, canonical_buff, out_length) == 0);
+  printf("Concat result: '%s'\n", out_buff);
+  
+  rc= mysql_stmt_fetch(stmt);
+  assert(rc == MYSQL_NO_DATA);
+
+  mysql_stmt_close(stmt);
+
+  stmt_text= "DROP TABLE IF EXISTS t1";
+  rc= mysql_real_query(mysql, stmt_text, strlen(stmt_text));
+  myquery(rc);
 }
 
 /*
@@ -10164,6 +10247,7 @@ int main(int argc, char **argv)
     test_order_param();	    /* ORDER BY with parameters in select list
 			       (Bug #3686 */
     test_ps_i18n();         /* test for i18n support in binary protocol */
+    test_bug3796();         /* test for select concat(?, <string>) */
 
     end_time= time((time_t *)0);
     total_time+= difftime(end_time, start_time);