Thrift
  1. Thrift
  2. THRIFT-1389

c_glib_generator.cc generates leaking code for cassandra_client_get_slice() and cassandra_client_get()

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.6.1
    • Fix Version/s: 0.9.2
    • Labels:
      None
    • Environment:

      Used with Cassandra 0.8.4 on Linux 2.6.32 with x86_64

      Description

      The generated cassandra.c cassandra_client_recv_get_slice() code for list processing instantiates an object of TYPE_COLUMN_OR_SUPER_COLUMN then calls thrift_struct_read() as in the following code snippet:

            switch (fid)
            {
              case 0:
                if (ftype == T_LIST)
                {
                  {
                    guint32 size;
                    ThriftType element_type;
                    if ((ret = thrift_protocol_read_list_begin (protocol, &element_type, &size, error)) < 0)
                      return 0;
                    xfer += ret;
      
                    /* iterate through list elements */
                    guint32 i;
                    for (i = 0; i < size; i++)
                    {
                      ColumnOrSuperColumn * _elem19;
                      _elem19 = g_object_new (TYPE_COLUMN_OR_SUPER_COLUMN, NULL);
                      //printf("\nrecv_slice made %p\n", _elem19->column);
                      if ((ret = thrift_struct_read (THRIFT_STRUCT (_elem19), protocol, error)) < 0)
      

      When the object of TYPE_COLUMN_OR_SUPER_COLUMN is instantiated, it in turn instantiates a child of TYPE_COLUMN:

      void
      column_or_super_column_instance_init (ColumnOrSuperColumn * object)
      {
        /* satisfy -Wall */
        THRIFT_UNUSED_VAR (object);
        object->column = g_object_new (TYPE_COLUMN, NULL);
        object->__isset_column = FALSE;
      

      But this instance reference is lost and replaced by a new TYPE_COLUMN instantiation reference when the column member is read by column_or_super_column_read() within the same execution context of the top level cassandra_client_recv_get_slice() call:

          switch (fid)
          {
            case 1:
              if (ftype == T_STRUCT)
              {
      
                this_object->column = g_object_new (TYPE_COLUMN, NULL);
                if ((ret = thrift_struct_read (THRIFT_STRUCT (this_object->column), protocol, error)) < 0)
      

      The above snippets/logic/leak described above for cassandra_client_get_slice() also apply for cassandra_client_get().

        Activity

        Hide
        Jerry Gally added a comment -

        The following valgrind leak reports initially exposed the leaks, and manually placed printf's in the generated cassandra code demonstrated column instantiation followed by re-instantiation and reference lost upon read:

        ==5051== 9,072 bytes in 126 blocks are definitely lost in loss record 527 of 528
        ==5051== at 0x4C2110C: malloc (vg_replace_malloc.c:195)
        ==5051== by 0x52ABBFA: g_malloc (in /lib64/libglib-2.0.so.0.1200.3)
        ==5051== by 0x52BB305: g_slice_alloc0 (in /lib64/libglib-2.0.so.0.1200.3)
        ==5051== by 0x50617CE: g_type_create_instance (in /lib64/libgobject-2.0.so.0.1200.3)
        ==5051== by 0x50498BC: ??? (in /lib64/libgobject-2.0.so.0.1200.3)
        ==5051== by 0x5047723: g_object_newv (in /lib64/libgobject-2.0.so.0.1200.3)
        ==5051== by 0x504814B: g_object_new_valist (in /lib64/libgobject-2.0.so.0.1200.3)
        ==5051== by 0x5048380: g_object_new (in /lib64/libgobject-2.0.so.0.1200.3)
        ==5051== by 0x41E6BB: column_or_super_column_instance_init (cassandra_types.c:1148)
        ==5051== by 0x5061C8F: g_type_create_instance (in /lib64/libgobject-2.0.so.0.1200.3)
        ==5051== by 0x50498BC: ??? (in /lib64/libgobject-2.0.so.0.1200.3)
        ==5051== by 0x5047723: g_object_newv (in /lib64/libgobject-2.0.so.0.1200.3)
        ==5051== by 0x504814B: g_object_new_valist (in /lib64/libgobject-2.0.so.0.1200.3)
        ==5051== by 0x5048380: g_object_new (in /lib64/libgobject-2.0.so.0.1200.3)
        ==5051== by 0x409E2C: cassandra_client_recv_get (cassandra.c:792)
        ==5051== by 0x40A3A6: cassandra_client_get (cassandra.c:904)
        ==5051== by 0x40723E: CregListNodeRecursively (sampleclient.c:952)
        ==5051== by 0x4070EB: CregListNodeRecursively (sampleclient.c:931)
        ==5051== by 0x4070EB: CregListNodeRecursively (sampleclient.c:931)
        ==5051== by 0x406D8E: CregListNodes (sampleclient.c:842)
        ==5051== by 0x4078FD: main (sampleclient.c:1133)

        ==5051== 21,168 bytes in 294 blocks are definitely lost in loss record 528 of 528
        ==5051== at 0x4C2110C: malloc (vg_replace_malloc.c:195)
        ==5051== by 0x52ABBFA: g_malloc (in /lib64/libglib-2.0.so.0.1200.3)
        ==5051== by 0x52BB305: g_slice_alloc0 (in /lib64/libglib-2.0.so.0.1200.3)
        ==5051== by 0x50617CE: g_type_create_instance (in /lib64/libgobject-2.0.so.0.1200.3)
        ==5051== by 0x50498BC: ??? (in /lib64/libgobject-2.0.so.0.1200.3)
        ==5051== by 0x5047723: g_object_newv (in /lib64/libgobject-2.0.so.0.1200.3)
        ==5051== by 0x504814B: g_object_new_valist (in /lib64/libgobject-2.0.so.0.1200.3)
        ==5051== by 0x5048380: g_object_new (in /lib64/libgobject-2.0.so.0.1200.3)
        ==5051== by 0x41E6BB: column_or_super_column_instance_init (cassandra_types.c:1148)
        ==5051== by 0x5061C8F: g_type_create_instance (in /lib64/libgobject-2.0.so.0.1200.3)
        ==5051== by 0x50498BC: ??? (in /lib64/libgobject-2.0.so.0.1200.3)
        ==5051== by 0x5047723: g_object_newv (in /lib64/libgobject-2.0.so.0.1200.3)
        ==5051== by 0x504814B: g_object_new_valist (in /lib64/libgobject-2.0.so.0.1200.3)
        ==5051== by 0x5048380: g_object_new (in /lib64/libgobject-2.0.so.0.1200.3)
        ==5051== by 0x40AB90: cassandra_client_recv_get_slice (cassandra.c:1078)
        ==5051== by 0x40B0A1: cassandra_client_get_slice (cassandra.c:1179)
        ==5051== by 0x406F58: CregListNodeRecursively (sampleclient.c:893)
        ==5051== by 0x4070EB: CregListNodeRecursively (sampleclient.c:931)
        ==5051== by 0x4070EB: CregListNodeRecursively (sampleclient.c:931)
        ==5051== by 0x406D8E: CregListNodes (sampleclient.c:842)
        ==5051== by 0x4078FD: main (sampleclient.c:1133)

        Show
        Jerry Gally added a comment - The following valgrind leak reports initially exposed the leaks, and manually placed printf's in the generated cassandra code demonstrated column instantiation followed by re-instantiation and reference lost upon read: ==5051== 9,072 bytes in 126 blocks are definitely lost in loss record 527 of 528 ==5051== at 0x4C2110C: malloc (vg_replace_malloc.c:195) ==5051== by 0x52ABBFA: g_malloc (in /lib64/libglib-2.0.so.0.1200.3) ==5051== by 0x52BB305: g_slice_alloc0 (in /lib64/libglib-2.0.so.0.1200.3) ==5051== by 0x50617CE: g_type_create_instance (in /lib64/libgobject-2.0.so.0.1200.3) ==5051== by 0x50498BC: ??? (in /lib64/libgobject-2.0.so.0.1200.3) ==5051== by 0x5047723: g_object_newv (in /lib64/libgobject-2.0.so.0.1200.3) ==5051== by 0x504814B: g_object_new_valist (in /lib64/libgobject-2.0.so.0.1200.3) ==5051== by 0x5048380: g_object_new (in /lib64/libgobject-2.0.so.0.1200.3) ==5051== by 0x41E6BB: column_or_super_column_instance_init (cassandra_types.c:1148) ==5051== by 0x5061C8F: g_type_create_instance (in /lib64/libgobject-2.0.so.0.1200.3) ==5051== by 0x50498BC: ??? (in /lib64/libgobject-2.0.so.0.1200.3) ==5051== by 0x5047723: g_object_newv (in /lib64/libgobject-2.0.so.0.1200.3) ==5051== by 0x504814B: g_object_new_valist (in /lib64/libgobject-2.0.so.0.1200.3) ==5051== by 0x5048380: g_object_new (in /lib64/libgobject-2.0.so.0.1200.3) ==5051== by 0x409E2C: cassandra_client_recv_get (cassandra.c:792) ==5051== by 0x40A3A6: cassandra_client_get (cassandra.c:904) ==5051== by 0x40723E: CregListNodeRecursively (sampleclient.c:952) ==5051== by 0x4070EB: CregListNodeRecursively (sampleclient.c:931) ==5051== by 0x4070EB: CregListNodeRecursively (sampleclient.c:931) ==5051== by 0x406D8E: CregListNodes (sampleclient.c:842) ==5051== by 0x4078FD: main (sampleclient.c:1133) ==5051== 21,168 bytes in 294 blocks are definitely lost in loss record 528 of 528 ==5051== at 0x4C2110C: malloc (vg_replace_malloc.c:195) ==5051== by 0x52ABBFA: g_malloc (in /lib64/libglib-2.0.so.0.1200.3) ==5051== by 0x52BB305: g_slice_alloc0 (in /lib64/libglib-2.0.so.0.1200.3) ==5051== by 0x50617CE: g_type_create_instance (in /lib64/libgobject-2.0.so.0.1200.3) ==5051== by 0x50498BC: ??? (in /lib64/libgobject-2.0.so.0.1200.3) ==5051== by 0x5047723: g_object_newv (in /lib64/libgobject-2.0.so.0.1200.3) ==5051== by 0x504814B: g_object_new_valist (in /lib64/libgobject-2.0.so.0.1200.3) ==5051== by 0x5048380: g_object_new (in /lib64/libgobject-2.0.so.0.1200.3) ==5051== by 0x41E6BB: column_or_super_column_instance_init (cassandra_types.c:1148) ==5051== by 0x5061C8F: g_type_create_instance (in /lib64/libgobject-2.0.so.0.1200.3) ==5051== by 0x50498BC: ??? (in /lib64/libgobject-2.0.so.0.1200.3) ==5051== by 0x5047723: g_object_newv (in /lib64/libgobject-2.0.so.0.1200.3) ==5051== by 0x504814B: g_object_new_valist (in /lib64/libgobject-2.0.so.0.1200.3) ==5051== by 0x5048380: g_object_new (in /lib64/libgobject-2.0.so.0.1200.3) ==5051== by 0x40AB90: cassandra_client_recv_get_slice (cassandra.c:1078) ==5051== by 0x40B0A1: cassandra_client_get_slice (cassandra.c:1179) ==5051== by 0x406F58: CregListNodeRecursively (sampleclient.c:893) ==5051== by 0x4070EB: CregListNodeRecursively (sampleclient.c:931) ==5051== by 0x4070EB: CregListNodeRecursively (sampleclient.c:931) ==5051== by 0x406D8E: CregListNodes (sampleclient.c:842) ==5051== by 0x4078FD: main (sampleclient.c:1133)
        Hide
        Jerry Gally added a comment -

        The following change to t_c_glib_generator.cc appears to fix the leak:

        1733c1733
        < indent(f_types_impl_) << "object->" << name << " = g_object_new (" << this->nspace_uc << "TYPE_" << type_name_uc << ", NULL);" << endl;

        > indent(f_types_impl_) << "object->" << name << " = NULL;" << endl;

        Show
        Jerry Gally added a comment - The following change to t_c_glib_generator.cc appears to fix the leak: 1733c1733 < indent(f_types_impl_) << "object->" << name << " = g_object_new (" << this->nspace_uc << "TYPE_" << type_name_uc << ", NULL);" << endl; — > indent(f_types_impl_) << "object->" << name << " = NULL;" << endl;
        Hide
        Andris Mednis added a comment -

        The fix in t_c_glib_generator.cc proposed by Jerry Gally helped to fix memory leak when Thrift 0.7.0 was used to generate files for connecting to Cassandra 1.0.7 from a C application. Does anybody know why the fix is not included in Thrift?

        Show
        Andris Mednis added a comment - The fix in t_c_glib_generator.cc proposed by Jerry Gally helped to fix memory leak when Thrift 0.7.0 was used to generate files for connecting to Cassandra 1.0.7 from a C application. Does anybody know why the fix is not included in Thrift?
        Hide
        Jerry Gally added a comment -

        When I ran accross this issue last year, and went searching for kindred spirits, I was left with the distinct impression that the C Thrift community was, as far as I could tell, virtually non-existent beyond the original _c_glib_generator.cc developer/contributer.

        Recieving your follow-up question on this issue is kind of like detecting a radio signal in the Search for Extra-Terrestrial Intellgence (SETI) program

        I, on the other hand, am no longer delving into the C Thrift context, but I do hope you find kindred spirits 8-)

        Show
        Jerry Gally added a comment - When I ran accross this issue last year, and went searching for kindred spirits, I was left with the distinct impression that the C Thrift community was, as far as I could tell, virtually non-existent beyond the original _c_glib_generator.cc developer/contributer. Recieving your follow-up question on this issue is kind of like detecting a radio signal in the Search for Extra-Terrestrial Intellgence (SETI) program I, on the other hand, am no longer delving into the C Thrift context, but I do hope you find kindred spirits 8-)
        Hide
        Simon South added a comment -

        I believe this issue can be closed. It was apparently fixed as part of THRIFT-1583, "c_glib leaks memory" (in this commit). t_c_glib_generator::generate_deserialize_struct was modified so the generated code no longer creates a new object (a Column, in this case) but rather invokes thrift_struct_read on the one already created by the containing object's constructor.

        Here's the relevant portion of the code generated for column_or_super_column_read by the compiler today:

            switch (fid)
            {
              case 1:
                if (ftype == T_STRUCT)
                {
                  if ((ret = thrift_struct_read (THRIFT_STRUCT (this_object->column), protocol, error)) < 0)
                  ...
        

        No superfluous call to g_object_new and hence no memory leak.

        I'll respond to Andris' comment above as it may be relevant to future contributors:

        The change proposed by Jerry addresses the memory leak in this case, but does so by modifying the containing object's constructor so it does not initialize its member Column object during construction. This violates a principle observed throughout the rest of the C implementation, which is that serializable objects are initialized as early as possible.

        This is done (I believe) because Thrift itself has no notion of a null object. If, for instance, a service method is defined as returning a map but for a particular invocation the method has no data to return, what the client receives is an empty map, not a null pointer or similar. As a result there is no reason for a serializable object or its members to ever not be initialized, as there is no way of representing them within the Thrift protocol otherwise. (This is my understanding, at least—perhaps someone more knowledgeable will correct me.)

        So with Jerry's fix applied, newly constructed ColumnOrSuperColumn objects cannot be serialized at all. The correct fix was to stop the generated code from blindly re-initializing serializable objects' members; the compiler should know they are always initialized at construction.

        Show
        Simon South added a comment - I believe this issue can be closed. It was apparently fixed as part of THRIFT-1583, "c_glib leaks memory" (in this commit ). t_c_glib_generator::generate_deserialize_struct was modified so the generated code no longer creates a new object (a Column , in this case) but rather invokes thrift_struct_read on the one already created by the containing object's constructor. Here's the relevant portion of the code generated for column_or_super_column_read by the compiler today: switch (fid) { case 1: if (ftype == T_STRUCT) { if ((ret = thrift_struct_read (THRIFT_STRUCT (this_object->column), protocol, error)) < 0) ... No superfluous call to g_object_new and hence no memory leak. I'll respond to Andris' comment above as it may be relevant to future contributors: The change proposed by Jerry addresses the memory leak in this case, but does so by modifying the containing object's constructor so it does not initialize its member Column object during construction. This violates a principle observed throughout the rest of the C implementation, which is that serializable objects are initialized as early as possible . This is done (I believe) because Thrift itself has no notion of a null object. If, for instance, a service method is defined as returning a map but for a particular invocation the method has no data to return, what the client receives is an empty map , not a null pointer or similar. As a result there is no reason for a serializable object or its members to ever not be initialized, as there is no way of representing them within the Thrift protocol otherwise. (This is my understanding, at least—perhaps someone more knowledgeable will correct me.) So with Jerry's fix applied, newly constructed ColumnOrSuperColumn objects cannot be serialized at all. The correct fix was to stop the generated code from blindly re-initializing serializable objects' members; the compiler should know they are always initialized at construction.
        Hide
        Jake Farrell added a comment -

        Closing per comments

        Show
        Jake Farrell added a comment - Closing per comments

          People

          • Assignee:
            Simon South
            Reporter:
            Jerry Gally
          • Votes:
            1 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development