andrey          Thu May 28 16:35:17 2009 UTC

  Modified files:              
    /php-src/ext/mysqlnd        mysqlnd_palloc.c mysqlnd_ps.c 
                                mysqlnd_ps_codec.c mysqlnd_result.c 
  Log:
  Fix a problem with cursors, which did not happen with unbuffered PS for
  some reason. Double free of the data, which led to valgrind warnigns.
  The fix actually optimizes the code in this cases because the old code
  used copy_ctor while the new one skips it because it is not needed.
  Transferring data ownership and nulling works best, for PS where we
  always copy the string from the result set, unlike the text protocol.
  
  
http://cvs.php.net/viewvc.cgi/php-src/ext/mysqlnd/mysqlnd_palloc.c?r1=1.17&r2=1.18&diff_format=u
Index: php-src/ext/mysqlnd/mysqlnd_palloc.c
diff -u php-src/ext/mysqlnd/mysqlnd_palloc.c:1.17 
php-src/ext/mysqlnd/mysqlnd_palloc.c:1.18
--- php-src/ext/mysqlnd/mysqlnd_palloc.c:1.17   Fri Mar 27 19:50:56 2009
+++ php-src/ext/mysqlnd/mysqlnd_palloc.c        Thu May 28 16:35:16 2009
@@ -18,7 +18,7 @@
   +----------------------------------------------------------------------+
 */
 
-/* $Id: mysqlnd_palloc.c,v 1.17 2009/03/27 19:50:56 felipe Exp $ */
+/* $Id: mysqlnd_palloc.c,v 1.18 2009/05/28 16:35:16 andrey Exp $ */
 #include "php.h"
 #include "mysqlnd.h"
 #include "mysqlnd_priv.h"
@@ -497,9 +497,14 @@
                *(thd_cache->gc_list.last_added++) = (mysqlnd_zval *)*zv;
                UNLOCK_PCACHE(cache);
        } else {
+               DBG_INF("No user reference");
                /* No user reference */
                if (((mysqlnd_zval *)*zv)->point_type == 
MYSQLND_POINTS_EXT_BUFFER) {
-                       /* PS are here and also in Unicode mode, for non-binary 
 */
+                       DBG_INF("Points to external buffer. Calling zval_dtor");
+                       /*
+                         PS are here
+                         Unicode mode goes also here if the column is not 
binary but a text
+                       */
                        zval_dtor(*zv);
                }
                LOCK_PCACHE(cache);
http://cvs.php.net/viewvc.cgi/php-src/ext/mysqlnd/mysqlnd_ps.c?r1=1.27&r2=1.28&diff_format=u
Index: php-src/ext/mysqlnd/mysqlnd_ps.c
diff -u php-src/ext/mysqlnd/mysqlnd_ps.c:1.27 
php-src/ext/mysqlnd/mysqlnd_ps.c:1.28
--- php-src/ext/mysqlnd/mysqlnd_ps.c:1.27       Mon Feb 16 17:25:37 2009
+++ php-src/ext/mysqlnd/mysqlnd_ps.c    Thu May 28 16:35:16 2009
@@ -18,7 +18,7 @@
   +----------------------------------------------------------------------+
 */
 
-/* $Id: mysqlnd_ps.c,v 1.27 2009/02/16 17:25:37 johannes Exp $ */
+/* $Id: mysqlnd_ps.c,v 1.28 2009/05/28 16:35:16 andrey Exp $ */
 #include "php.h"
 #include "mysqlnd.h"
 #include "mysqlnd_wireprotocol.h"
@@ -536,12 +536,14 @@
                                                
stmt->upsert_status.server_status & SERVER_STATUS_CURSOR_EXISTS);
 
                        if (stmt->upsert_status.server_status & 
SERVER_STATUS_CURSOR_EXISTS) {
+                               DBG_INF("cursor exists");
                                stmt->cursor_exists = TRUE;
                                CONN_SET_STATE(conn, CONN_READY);
                                /* Only cursor read */
                                stmt->default_rset_handler = 
stmt->m->use_result;
                                DBG_INF("use_result");
                        } else if (stmt->flags & CURSOR_TYPE_READ_ONLY) {
+                               DBG_INF("asked for cursor but got none");
                                /*
                                  We have asked for CURSOR but got no cursor, 
because the condition
                                  above is not fulfilled. Then...
@@ -556,6 +558,7 @@
                                stmt->default_rset_handler = 
stmt->m->store_result;
                                DBG_INF("store_result");
                        } else {
+                               DBG_INF("no cursor");
                                /* preferred is unbuffered read */
                                stmt->default_rset_handler = 
stmt->m->use_result;
                                DBG_INF("use_result");
@@ -853,15 +856,10 @@
                                          stmt->result_bind[i].zv has been 
already destructed
                                          in mysqlnd_unbuffered_free_last_data()
                                        */
-
 #ifndef WE_DONT_COPY_IN_BUFFERED_AND_UNBUFFERED_BECAUSEOF_IS_REF
                                        zval_dtor(stmt->result_bind[i].zv);
 #endif
                                        if (IS_NULL != 
(Z_TYPE_P(stmt->result_bind[i].zv) = Z_TYPE_P(data)) ) {
-                                               stmt->result_bind[i].zv->value 
= data->value;
-#ifndef WE_DONT_COPY_IN_BUFFERED_AND_UNBUFFERED_BECAUSEOF_IS_REF
-                                               
zval_copy_ctor(stmt->result_bind[i].zv);
-#endif                                         
                                                if (
                                                        (Z_TYPE_P(data) == 
IS_STRING
 #if PHP_MAJOR_VERSION >= 6
@@ -872,6 +870,9 @@
                                                {
                                                        
result->meta->fields[i].max_length = Z_STRLEN_P(data);
                                                }
+                                               stmt->result_bind[i].zv->value 
= data->value;
+                                               // copied data, thus also the 
ownership. Thus null data
+                                               ZVAL_NULL(data);
                                        }
                                }
                        }
@@ -888,14 +889,15 @@
                        row_packet->row_buffer = NULL;
                }
        } else if (ret == FAIL) {
+               *fetched_anything = FALSE;
                if (row_packet->error_info.error_no) {
                        stmt->conn->error_info = row_packet->error_info; 
                        stmt->error_info = row_packet->error_info; 
                }
-               *fetched_anything = FALSE;
                CONN_SET_STATE(result->conn, CONN_READY);
                result->unbuf->eof_reached = TRUE; /* so next time we won't get 
an error */     
        } else if (row_packet->eof) {
+               *fetched_anything = FALSE;
                DBG_INF("EOF");
                /* Mark the connection as usable again */
                result->unbuf->eof_reached = TRUE;
@@ -910,7 +912,6 @@
                } else {
                        CONN_SET_STATE(result->conn, CONN_READY);
                }
-               *fetched_anything = FALSE;
        }
 
        DBG_INF_FMT("ret=%s fetched_anything=%d", ret == PASS? "PASS":"FAIL", 
*fetched_anything);
@@ -944,7 +945,8 @@
 
        MYSQLND_INC_CONN_STATISTIC(&stmt->conn->stats, STAT_PS_UNBUFFERED_SETS);
        result = stmt->result;
-       
+
+       DBG_INF_FMT("%scursor exists", stmt->cursor_exists? "":"no ");
        result->m.use_result(stmt->result, TRUE TSRMLS_CC);
        result->m.fetch_row     = stmt->cursor_exists? 
mysqlnd_fetch_stmt_row_cursor:
                                                                                
           mysqlnd_stmt_fetch_row_unbuffered;
@@ -1002,15 +1004,19 @@
 
        if (PASS == (ret = PACKET_READ(row_packet, result->conn)) && 
!row_packet->eof) {
                unsigned int i, field_count = result->field_count;
-               mysqlnd_unbuffered_free_last_data(result TSRMLS_CC);
+               result->unbuf->row_count++;
+               *fetched_anything = TRUE;
 
-               result->unbuf->last_row_data = row_packet->fields;
-               result->unbuf->last_row_buffer = row_packet->row_buffer;
+               DBG_INF_FMT("skip_extraction=%d", row_packet->skip_extraction); 
+               if (!row_packet->skip_extraction) {
+                       mysqlnd_unbuffered_free_last_data(result TSRMLS_CC);
 
+                       DBG_INF("extracting data");
+                       result->unbuf->last_row_data = row_packet->fields;
+                       result->unbuf->last_row_buffer = row_packet->row_buffer;
+                       row_packet->fields = NULL;
+                       row_packet->row_buffer = NULL;
 
-               row_packet->fields = NULL;
-               row_packet->row_buffer = NULL;
-               if (!row_packet->skip_extraction) {
                        result->m.row_decoder(result->unbuf->last_row_buffer,
                                                                  
result->unbuf->last_row_data,
                                                                  
row_packet->field_count,
@@ -1025,12 +1031,12 @@
                                          stmt->result_bind[i].zv has been 
already destructed
                                          in mysqlnd_unbuffered_free_last_data()
                                        */
-                                       DBG_INF_FMT("i=%d type=%d", i, 
Z_TYPE_P(stmt->result_bind[i].zv));
-                                       if (IS_NULL != 
(Z_TYPE_P(stmt->result_bind[i].zv) = Z_TYPE_P(data)) ) {
-                                               stmt->result_bind[i].zv->value 
= data->value;
-#ifdef WE_DONT_COPY_IN_BUFFERED_AND_UNBUFFERED_BECAUSEOF_IS_REF
-                                               
zval_copy_ctor(stmt->result_bind[i].zv);
+#ifndef WE_DONT_COPY_IN_BUFFERED_AND_UNBUFFERED_BECAUSEOF_IS_REF
+                                       zval_dtor(stmt->result_bind[i].zv);
 #endif
+                                       DBG_INF_FMT("i=%d bound_var=%p type=%d 
refc=%u", i, stmt->result_bind[i].zv,
+                                                               Z_TYPE_P(data), 
Z_REFCOUNT_P(stmt->result_bind[i].zv));
+                                       if (IS_NULL != 
(Z_TYPE_P(stmt->result_bind[i].zv) = Z_TYPE_P(data))) {
                                                if ((Z_TYPE_P(data) == IS_STRING
 #if PHP_MAJOR_VERSION >= 6
                                                        || Z_TYPE_P(data) == 
IS_UNICODE
@@ -1040,12 +1046,23 @@
                                                {
                                                        
result->meta->fields[i].max_length = Z_STRLEN_P(data);
                                                }
+                                               stmt->result_bind[i].zv->value 
= data->value;
+                                               // copied data, thus also the 
ownership. Thus null data
+                                               ZVAL_NULL(data);
                                        }
                                }
                        }
+               } else {
+                       DBG_INF("skipping extraction");
+                       /*
+                         Data has been allocated and usually 
mysqlnd_unbuffered_free_last_data()
+                         frees it but we can't call this function as it will 
cause problems with
+                         the bound variables. Thus we need to do part of what 
it does or Zend will
+                         report leaks.
+                       */
+                       
row_packet->row_buffer->free_chunk(row_packet->row_buffer, TRUE TSRMLS_CC);
+                       row_packet->row_buffer = NULL;
                }
-               result->unbuf->row_count++;
-               *fetched_anything = TRUE;
                /* We asked for one row, the next one should be EOF, eat it */
                ret = PACKET_READ(row_packet, result->conn);
                if (row_packet->row_buffer) {
@@ -1491,7 +1508,8 @@
                stmt->result_bind = result_bind;
                for (i = 0; i < stmt->field_count; i++) {
                        /* Prevent from freeing */
-                       Z_ADDREF_P(stmt->result_bind[i].zv);            
+                       Z_ADDREF_P(stmt->result_bind[i].zv);
+                       DBG_INF_FMT("ref of %p = %u", stmt->result_bind[i].zv, 
Z_REFCOUNT_P(stmt->result_bind[i].zv));
                        /*
                          Don't update is_ref !!! it's not our job
                          Otherwise either 009.phpt or 
mysqli_stmt_bind_result.phpt
http://cvs.php.net/viewvc.cgi/php-src/ext/mysqlnd/mysqlnd_ps_codec.c?r1=1.16&r2=1.17&diff_format=u
Index: php-src/ext/mysqlnd/mysqlnd_ps_codec.c
diff -u php-src/ext/mysqlnd/mysqlnd_ps_codec.c:1.16 
php-src/ext/mysqlnd/mysqlnd_ps_codec.c:1.17
--- php-src/ext/mysqlnd/mysqlnd_ps_codec.c:1.16 Fri Mar 27 19:28:26 2009
+++ php-src/ext/mysqlnd/mysqlnd_ps_codec.c      Thu May 28 16:35:16 2009
@@ -18,7 +18,7 @@
   +----------------------------------------------------------------------+
 */
 
-/* $Id: mysqlnd_ps_codec.c,v 1.16 2009/03/27 19:28:26 felipe Exp $ */
+/* $Id: mysqlnd_ps_codec.c,v 1.17 2009/05/28 16:35:16 andrey Exp $ */
 #include "php.h"
 #include "mysqlnd.h"
 #include "mysqlnd_wireprotocol.h"
@@ -405,12 +405,14 @@
        DBG_ENTER("ps_fetch_string");
        DBG_INF_FMT("len = %lu", length);
 #if PHP_MAJOR_VERSION < 6
+       DBG_INF("copying from the row buffer");
        ZVAL_STRINGL(zv, (char *)*row, length, 1);      
 #else
        if (field->charsetnr == MYSQLND_BINARY_CHARSET_NR) {
                DBG_INF("Binary charset");
                ZVAL_STRINGL(zv, (char *)*row, length, 1);
        } else {
+               DBG_INF_FMT("copying from the row buffer");
                ZVAL_UTF8_STRINGL(zv, (char*)*row, length, ZSTR_DUPLICATE);
        }
 #endif
http://cvs.php.net/viewvc.cgi/php-src/ext/mysqlnd/mysqlnd_result.c?r1=1.31&r2=1.32&diff_format=u
Index: php-src/ext/mysqlnd/mysqlnd_result.c
diff -u php-src/ext/mysqlnd/mysqlnd_result.c:1.31 
php-src/ext/mysqlnd/mysqlnd_result.c:1.32
--- php-src/ext/mysqlnd/mysqlnd_result.c:1.31   Thu May 28 11:47:15 2009
+++ php-src/ext/mysqlnd/mysqlnd_result.c        Thu May 28 16:35:16 2009
@@ -18,7 +18,7 @@
   +----------------------------------------------------------------------+
 */
 
-/* $Id: mysqlnd_result.c,v 1.31 2009/05/28 11:47:15 andrey Exp $ */
+/* $Id: mysqlnd_result.c,v 1.32 2009/05/28 16:35:16 andrey Exp $ */
 #include "php.h"
 #include "mysqlnd.h"
 #include "mysqlnd_wireprotocol.h"
@@ -143,10 +143,13 @@
                DBG_VOID_RETURN;
        }
 
+       DBG_INF_FMT("last_row_data=%p", unbuf->last_row_data);
        if (unbuf->last_row_data) {
                unsigned int i, ctor_called_count = 0;
                zend_bool copy_ctor_called;
                MYSQLND_STATS *global_stats = result->conn? 
&result->conn->stats:NULL;
+
+               DBG_INF_FMT("%u columns to free", result->field_count);
                for (i = 0; i < result->field_count; i++) {
                        mysqlnd_palloc_zval_ptr_dtor(&(unbuf->last_row_data[i]),
                                                                                
 result->zval_cache, result->type,
@@ -155,6 +158,7 @@
                                ctor_called_count++;
                        }
                }
+               DBG_INF_FMT("copy_ctor_called_count=%u", ctor_called_count);
                /* By using value3 macros we hold a mutex only once, there is 
no value2 */
                MYSQLND_INC_CONN_STATISTIC_W_VALUE3(global_stats,
                                                                                
        STAT_COPY_ON_WRITE_PERFORMED,
@@ -168,6 +172,7 @@
                unbuf->last_row_data = NULL;
        }
        if (unbuf->last_row_buffer) {
+               DBG_INF("Freeing last row buffer");
                /* Nothing points to this buffer now, free it */
                unbuf->last_row_buffer->free_chunk(unbuf->last_row_buffer, TRUE 
TSRMLS_CC);
                unbuf->last_row_buffer = NULL;

-- 
PHP CVS Mailing List (http://www.php.net/)
To unsubscribe, visit: http://www.php.net/unsub.php

Reply via email to