Update safety logic around packet in function.

configure.ac

@@ -138,6 +138,10 @@ AC_CHECK_HEADERS(errno.h getopt.h pwd.h signal.h)
 AC_CHECK_HEADERS(stdarg.h stddef.h stdio.h)
 AC_CHECK_HEADERS(sys/resource.h sys/stat.h)
 AC_CHECK_HEADERS(sys/socket.h sys/types.h sys/utsname.h strings.h)
+AC_CHECK_HEADERS([execinfo.h])
+AC_CHECK_HEADERS([cxxabi.h], 
+                 AC_DEFINE([HAVE_CXXABI_H], [1], [Have cxxabi.h]),
+                 AC_DEFINE([HAVE_CXXABI_H], [0], [Have cxxabi.h]))
 
 # Checks for typedefs, structures, and compiler characteristics.
 AC_HEADER_STDBOOL

libgearman-1.0/packet.h

@@ -68,4 +68,7 @@ struct gearman_packet_st
   char *arg[GEARMAN_MAX_COMMAND_ARGS];
   size_t arg_size[GEARMAN_MAX_COMMAND_ARGS];
   char args_buffer[GEARMAN_ARGS_BUFFER_SIZE];
+#ifdef GEARMAN_PACKET_TRACE
+  uint32_t _id;
+#endif
 };

libgearman/backtrace.cc

@@ -0,0 +1,146 @@
+/*  vim:expandtab:shiftwidth=2:tabstop=2:smarttab:
+ * 
+ *  libgearman client library.
+ *
+ *  Copyright (C) 2012 Data Differential, http://datadifferential.com/
+ *  All rights reserved.
+ *
+ *  Redistribution and use in source and binary forms, with or without
+ *  modification, are permitted provided that the following conditions are
+ *  met:
+ *
+ *      * Redistributions of source code must retain the above copyright
+ *  notice, this list of conditions and the following disclaimer.
+ *
+ *      * Redistributions in binary form must reproduce the above
+ *  copyright notice, this list of conditions and the following disclaimer
+ *  in the documentation and/or other materials provided with the
+ *  distribution.
+ *
+ *      * The names of its contributors may not be used to endorse or
+ *  promote products derived from this software without specific prior
+ *  written permission.
+ *
+ *  THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *  "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *  LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *  A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *  OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *  SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *  LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *  DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *  THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *  (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *  OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ */
+
+#include <config.h>
+#include <libgearman/common.h>
+
+#include <cstring>
+#include <cstdlib>
+
+#ifdef HAVE_EXECINFO_H
+#include <execinfo.h>
+#endif
+
+#ifdef HAVE_CXXABI_H
+#include <cxxabi.h>
+#endif
+
+#ifdef HAVE_GCC_ABI_DEMANGLE
+# define USE_DEMANGLE 1
+#else
+# define USE_DEMANGLE 0
+#endif
+
+void custom_backtrace(void)
+{
+#ifdef HAVE_EXECINFO_H
+  void *array[50];
+
+  size_t size= backtrace(array, 50);
+  char **strings= backtrace_symbols(array, size);
+
+  if (strings == NULL)
+  {
+    return;
+  }
+
+  fprintf(stderr, "Number of stack frames obtained: %lu\n", (unsigned long)size);
+
+  char *named_function= (char *)::realloc(NULL, 1024);
+  
+  if (named_function == NULL)
+  {
+    ::free(strings);
+    return;
+  }
+
+  for (size_t x= 1; x < size; x++) 
+  {
+    if (USE_DEMANGLE)
+    {
+      size_t sz= 200;
+      char *named_function_ptr= (char *)::realloc(named_function, sz);
+      if (named_function_ptr == NULL)
+      {
+        continue;
+      }
+      named_function= named_function_ptr;
+
+      char *begin_name= 0;
+      char *begin_offset= 0;
+      char *end_offset= 0;
+
+      for (char *j= strings[x]; *j; ++j)
+      {
+        if (*j == '(')
+        {
+          begin_name= j;
+        }
+        else if (*j == '+')
+        {
+          begin_offset= j;
+        }
+        else if (*j == ')' and begin_offset) 
+        {
+          end_offset= j;
+          break;
+        }
+      }
+
+      if (begin_name and begin_offset and end_offset and begin_name < begin_offset)
+      {
+        *begin_name++= '\0';
+        *begin_offset++= '\0';
+        *end_offset= '\0';
+
+        int status;
+        char *ret= abi::__cxa_demangle(begin_name, named_function, &sz, &status);
+        if (ret) // realloc()'ed string
+        {
+          named_function= ret;
+          fprintf(stderr, "  %s : %s()+%s\n", strings[x], begin_name, begin_offset);
+        }
+        else
+        {
+          fprintf(stderr, "  %s : %s()+%s\n", strings[x], begin_name, begin_offset);
+        }
+      }
+      else
+      {
+        fprintf(stderr, " %s\n", strings[x]);
+      }
+    }
+    else
+    {
+      fprintf(stderr, " unmangled: %s\n", strings[x]);
+    }
+  }
+
+  ::free(named_function);
+  ::free(strings);
+#endif // HAVE_EXECINFO_H
+}

libgearman/backtrace.hpp

@@ -0,0 +1,40 @@
+/*  vim:expandtab:shiftwidth=2:tabstop=2:smarttab:
+ * 
+ *  libgearman client library.
+ *
+ *  Copyright (C) 2012 Data Differential, http://datadifferential.com/
+ *  All rights reserved.
+ *
+ *  Redistribution and use in source and binary forms, with or without
+ *  modification, are permitted provided that the following conditions are
+ *  met:
+ *
+ *      * Redistributions of source code must retain the above copyright
+ *  notice, this list of conditions and the following disclaimer.
+ *
+ *      * Redistributions in binary form must reproduce the above
+ *  copyright notice, this list of conditions and the following disclaimer
+ *  in the documentation and/or other materials provided with the
+ *  distribution.
+ *
+ *      * The names of its contributors may not be used to endorse or
+ *  promote products derived from this software without specific prior
+ *  written permission.
+ *
+ *  THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *  "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *  LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *  A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *  OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *  SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *  LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *  DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *  THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *  (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *  OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ */
+
+#pragma once
+
+void custom_backtrace(void);

libgearman/connection.cc

@@ -173,7 +173,8 @@ gearman_connection_st::gearman_connection_st(gearman_universal_st &universal_arg
   recv_buffer_size(0),
   recv_data_size(0),
   recv_data_offset(0),
-  universal(universal_arg)
+  universal(universal_arg),
+  _recv_packet(NULL)
 {
   options.ready= false;
   options.packet_in_use= false;
@@ -200,7 +201,6 @@ gearman_connection_st::gearman_connection_st(gearman_universal_st &universal_arg
   addrinfo= NULL;
   addrinfo_next= NULL;
   send_buffer_ptr= send_buffer;
-  recv_packet= NULL;
   recv_buffer_ptr= recv_buffer;
   host[0]= 0;
 }
@@ -386,16 +386,21 @@ void gearman_connection_st::close_socket()
   send_data_offset= 0;
 
   recv_state= GEARMAN_CON_RECV_UNIVERSAL_NONE;
-  if (recv_packet)
-  {
-    gearman_packet_free(recv_packet);
-    recv_packet= NULL;
-  }
+  free_recv_packet();
 
   recv_buffer_ptr= recv_buffer;
   recv_buffer_size= 0;
 }
 
+void gearman_connection_st::free_recv_packet()
+{
+  if (_recv_packet)
+  {
+    gearman_packet_free(recv_packet());
+    _recv_packet= NULL;
+  }
+}
+
 void gearman_connection_st::reset_addrinfo()
 {
   if (addrinfo)
@@ -502,7 +507,7 @@ gearman_return_t gearman_connection_st::send_packet(const gearman_packet_st& pac
     send_data_size= packet_arg.data_size;
 
     /* If this is NULL, then gearman_connection_send_data function will be used. */
-    if (not packet_arg.data)
+    if (packet_arg.data == NULL)
     {
       send_state= GEARMAN_CON_SEND_UNIVERSAL_FLUSH_DATA;
       return GEARMAN_SUCCESS;
@@ -802,8 +807,8 @@ gearman_packet_st *gearman_connection_st::receiving(gearman_packet_st& packet_ar
       return NULL;
     }
 
-    recv_packet= gearman_packet_create(universal, &packet_arg);
-    if (recv_packet == NULL)
+    _recv_packet= gearman_packet_create(universal, &packet_arg);
+    if (_recv_packet == NULL)
     {
       ret= GEARMAN_MEMORY_ALLOCATION_FAILURE;
       return NULL;
@@ -816,7 +821,7 @@ gearman_packet_st *gearman_connection_st::receiving(gearman_packet_st& packet_ar
     {
       if (recv_buffer_size > 0)
       {
-        size_t recv_size= gearman_packet_unpack(*recv_packet,
+        size_t recv_size= gearman_packet_unpack(*(recv_packet()),
                                                 recv_buffer_ptr,
                                                 recv_buffer_size, ret);
         recv_buffer_ptr+= recv_size;
@@ -865,7 +870,7 @@ gearman_packet_st *gearman_connection_st::receiving(gearman_packet_st& packet_ar
 
     assert(packet_arg.universal);
     packet_arg.data= gearman_malloc((*packet_arg.universal), packet_arg.data_size);
-    if (not packet_arg.data)
+    if (packet_arg.data == NULL)
     {
       ret= GEARMAN_MEMORY_ALLOCATION_FAILURE;
       close_socket();
@@ -891,8 +896,8 @@ gearman_packet_st *gearman_connection_st::receiving(gearman_packet_st& packet_ar
     break;
   }
 
-  gearman_packet_st *tmp_packet_arg= recv_packet;
-  recv_packet= NULL;
+  gearman_packet_st *tmp_packet_arg= recv_packet();
+  set_recv_packet(NULL);
 
   return tmp_packet_arg;
 }

libgearman/connection.hpp

@@ -69,7 +69,6 @@ struct gearman_connection_st
   struct addrinfo *addrinfo;
   struct addrinfo *addrinfo_next;
   char *send_buffer_ptr;
-  gearman_packet_st *recv_packet;
   char *recv_buffer_ptr;
   gearman_packet_st _packet;
   char host[GEARMAN_NI_MAXHOST];
@@ -109,9 +108,21 @@ struct gearman_connection_st
 
   gearman_return_t lookup();
 
+  void free_recv_packet();
+  gearman_packet_st* recv_packet()
+  {
+    return _recv_packet;
+  }
+
+  void set_recv_packet(gearman_packet_st* arg)
+  {
+    _recv_packet= arg;
+  }
+
 private:
   size_t recv_socket(void *data, size_t data_size, gearman_return_t&);
   gearman_return_t connect_poll();
+  gearman_packet_st *_recv_packet;
 };
 
 /**

libgearman/function/base.hpp

@@ -68,7 +68,6 @@ struct _worker_function_st
   char *function_name;
   size_t function_length;
   void *context;
-  gearman_packet_st packet;
 
   _worker_function_st(void *context_arg) : 
     next(NULL),
@@ -117,8 +116,18 @@ struct _worker_function_st
   virtual ~_worker_function_st()
   {
     if (options.packet_in_use)
-      gearman_packet_free(&packet);
+    {
+      gearman_packet_free(&_packet);
+    }
 
     delete [] function_name;
   }
+
+  gearman_packet_st& packet()
+  {
+    return _packet;
+  }
+
+private:
+  gearman_packet_st _packet;
 };

libgearman/include.am

@@ -18,6 +18,7 @@ noinst_HEADERS+= \
 		 libgearman/aggregator.hpp \
 		 libgearman/allocator.hpp \
 		 libgearman/assert.hpp \
+		 libgearman/backtrace.hpp \
 		 libgearman/byteorder.h \
 		 libgearman/command.h \
 		 libgearman/common.h \
@@ -66,6 +67,7 @@ libgearman_libgearman_la_SOURCES= \
 				  libgearman/allocator.cc \
 				  libgearman/argument.cc \
 				  libgearman/add.cc \
+				  libgearman/backtrace.cc \
 				  libgearman/byteorder.cc \
 				  libgearman/client.cc \
 				  libgearman/command.cc \

libgearman/packet.cc

@@ -49,6 +49,8 @@
 #include <libgearman/command.h>
 #include <libgearman/packet.hpp>
 
+#include <libgearman/backtrace.hpp>
+
 #include <cerrno>
 #include <cstdlib>
 #include <cstring>
@@ -93,6 +95,7 @@ inline static gearman_return_t packet_create_arg(gearman_packet_st *packet,
   }
   else
   {
+    // If args is args_buffer we don't want to try realloc it
     if (packet->args == packet->args_buffer)
     {
       packet->args= NULL;
@@ -143,6 +146,12 @@ inline static gearman_return_t packet_create_arg(gearman_packet_st *packet,
  * Public Definitions
  */
 
+#ifdef GEARMAN_PACKET_TRACE
+#include <pthread.h>
+pthread_mutex_t mutex= PTHREAD_MUTEX_INITIALIZER;
+static uint32_t global_packet_id= 0;
+#endif
+
 gearman_packet_st *gearman_packet_create(gearman_universal_st &universal,
                                          gearman_packet_st *packet)
 {
@@ -164,6 +173,8 @@ gearman_packet_st *gearman_packet_create(gearman_universal_st &universal,
 
     packet->options.allocated= true;
   }
+  packet->options.complete= false;
+  packet->options.free_data= false;
 
   packet->magic= GEARMAN_MAGIC_TEXT;
   packet->command= GEARMAN_COMMAND_TEXT;
@@ -172,6 +183,14 @@ gearman_packet_st *gearman_packet_create(gearman_universal_st &universal,
   packet->data_size= 0;
   packet->universal= &universal;
 
+#ifdef GEARMAN_PACKET_TRACE
+  pthread_mutex_lock(&mutex);
+  packet->_id= global_packet_id++;
+  pthread_mutex_unlock(&mutex);
+  fprintf(stderr, "%s PACKET %u\n", __func__, packet->_id);
+  custom_backtrace();
+#endif
+
   if (universal.options.dont_track_packets == false)
   {
     if (universal.packet_list != NULL)
@@ -246,6 +265,10 @@ gearman_return_t gearman_packet_create_args(gearman_universal_st& universal,
 
 void gearman_packet_free(gearman_packet_st *packet)
 {
+#ifdef GEARMAN_PACKET_TRACE
+  fprintf(stderr, "%s PACKET %u\n", __func__, packet->_id);
+  custom_backtrace();
+#endif
   if (packet->args != packet->args_buffer and packet->args)
   {
     // Created with realloc
@@ -255,22 +278,29 @@ void gearman_packet_free(gearman_packet_st *packet)
 
   assert_msg(packet->universal, 
              "Packet that is being freed has not been allocated, most likely this is do to freeing a gearman_task_st or other object twice");
-  if (packet->options.free_data && packet->data)
+  if (packet->options.free_data and packet->data)
   {
     gearman_free((*packet->universal), const_cast<void *>(packet->data));
     packet->data= NULL;
+    packet->options.free_data= false;
   }
 
   if (packet->universal->options.dont_track_packets == false)
   {
     if (packet->universal->packet_list == packet)
+    {
       packet->universal->packet_list= packet->next;
+    }
 
     if (packet->prev)
+    {
       packet->prev->next= packet->next;
+    }
 
     if (packet->next)
+    {
       packet->next->prev= packet->prev;
+    }
 
     packet->universal->packet_count--;
   }

libgearman/universal.cc

@@ -463,7 +463,7 @@ gearman_return_t gearman_set_identifier(gearman_universal_st& universal,
       assert_msg(con->universal.error.rc != GEARMAN_SUCCESS, "Programmer error, error returned but not recorded");
 #endif
       con->free_private_packet();
-      con->recv_packet= NULL;
+      con->set_recv_packet(NULL);
 
       goto exit;
     }
@@ -476,13 +476,13 @@ gearman_return_t gearman_set_identifier(gearman_universal_st& universal,
       assert_msg(con->universal.error.rc != GEARMAN_SUCCESS, "Programmer error, error returned but not recorded");
 #endif
       con->free_private_packet();
-      con->recv_packet= NULL;
+      con->set_recv_packet(NULL);
       ret= gearman_error(universal, GEARMAN_ECHO_DATA_CORRUPTION, "corruption during echo");
 
       goto exit;
     }
 
-    con->recv_packet= NULL;
+    con->set_recv_packet(NULL);
     con->free_private_packet();
   }
 
@@ -550,7 +550,7 @@ gearman_return_t gearman_echo(gearman_universal_st& universal,
       assert_msg(con->universal.error.rc != GEARMAN_SUCCESS, "Programmer error, error returned but not recorded");
 #endif
       con->free_private_packet();
-      con->recv_packet= NULL;
+      con->set_recv_packet(NULL);
 
       goto exit;
     }
@@ -563,13 +563,13 @@ gearman_return_t gearman_echo(gearman_universal_st& universal,
       assert_msg(con->universal.error.rc != GEARMAN_SUCCESS, "Programmer error, error returned but not recorded");
 #endif
       con->free_private_packet();
-      con->recv_packet= NULL;
+      con->set_recv_packet(NULL);
       ret= gearman_error(universal, GEARMAN_ECHO_DATA_CORRUPTION, "corruption during echo");
 
       goto exit;
     }
 
-    con->recv_packet= NULL;
+    con->set_recv_packet(NULL);
     con->free_private_packet();
   }
 
@@ -620,7 +620,7 @@ bool gearman_request_option(gearman_universal_st &universal,
     }
     else if (gearman_failed(ret))
     {
-      con->recv_packet= NULL;
+      con->set_recv_packet(NULL);
       gearman_packet_free(&recv_packet);
       goto exit;
     }
@@ -628,7 +628,7 @@ bool gearman_request_option(gearman_universal_st &universal,
 
     if (packet_ptr->command == GEARMAN_COMMAND_ERROR)
     {
-      con->recv_packet= NULL;
+      con->set_recv_packet(NULL);
       gearman_packet_free(&recv_packet);
       ret= gearman_error(universal, GEARMAN_INVALID_ARGUMENT, "invalid server option");