Thrift
  1. Thrift
  2. THRIFT-1266

generated C code for iterating over nested maps is wrong

    Details

      Description

      We are working with Cassandra API in C generated by Thrift and have noticed a bug in the generated code for cassandra_client_send_batch_mutate().

      Full code that Thrift generates for this function is attached, but here is the specification for Cassandra's batch_mutate method:

      /**
        Mutate many columns or super columns for many row keys. See also: Mutation.
      
        mutation_map maps key to column family to a list of Mutation objects to take place at that scope.
      **/
      void batch_mutate(1:required map<binary, map<string, list<Mutation>>> mutation_map,
                        2:required ConsistencyLevel consistency_level=ConsistencyLevel.ONE)
           throws (1:InvalidRequestException ire, 2:UnavailableException ue, 3:TimedOutException te),
      

      If we now look at the generated code, we will notice the following fragment:

      GPtrArray * value;
      g_hash_table_foreach ((GHashTable *)  value, thrift_hash_table_get_keys, &key_list); /* LINE A */
      

      We can see that in line A it uses the variable "value" as GHashTable, even though the GHashTable "value" was shadowed by GPtrArray "value" a line before.

      Similarly, we can see another fragment below that one, where one instance of variable "value" shadows another instance:

      value = (GPtrArray *) g_hash_table_lookup (((GHashTable *)  value), (gpointer) key); /* LINE B */
      

      We have worked around the bug in our particular case by renaming one of the "value" variables to "value2" (see "svn di -c 21176 svn://svn.zabbix.com/branches/dev/ZBXNEXT-844/src/libs/zbxcassa/cassandra.c@21176" for a diff), but it would be nice to fix it in Thrift, too.

        Issue Links

          Activity

          Hide
          Roger Meier added a comment -

          could you please provide a test case if possible and a patch?

          see http://thrift.apache.org/docs/HowToContribute

          Show
          Roger Meier added a comment - could you please provide a test case if possible and a patch? see http://thrift.apache.org/docs/HowToContribute
          Hide
          Aleksandrs Saveljevs added a comment - - edited

          Unfortunately, neither a reduced test case, nor a patch would be possible, because none of us are experts on Thrift. We are simply Cassandra users and, as such, the bug should have been reported by Cassandra developers.

          However, I have just tried Thrift 0.8.0 on Cassandra 1.1.1. It generates the same code for batch_mutate() method, so it seems the problem has not been fixed yet and is easily reproducible.

          In order to reproduce the problem using Cassandra, you can do the following:
          (1) download Cassandra source from http://cassandra.apache.org/download/ ;
          (2) from Cassandra source root, run Thrift using "thrift --gen c_glib interface/cassandra.thrift";
          (3) open "gen-c_glib/cassandra.c" and observe that "value" variable is incorrectly shadowed inside cassandra_client_send_batch_mutate().

          Show
          Aleksandrs Saveljevs added a comment - - edited Unfortunately, neither a reduced test case, nor a patch would be possible, because none of us are experts on Thrift. We are simply Cassandra users and, as such, the bug should have been reported by Cassandra developers. However, I have just tried Thrift 0.8.0 on Cassandra 1.1.1. It generates the same code for batch_mutate() method, so it seems the problem has not been fixed yet and is easily reproducible. In order to reproduce the problem using Cassandra, you can do the following: (1) download Cassandra source from http://cassandra.apache.org/download/ ; (2) from Cassandra source root, run Thrift using "thrift --gen c_glib interface/cassandra.thrift"; (3) open "gen-c_glib/cassandra.c" and observe that "value" variable is incorrectly shadowed inside cassandra_client_send_batch_mutate().
          Hide
          Simon South added a comment -

          In the "better late than never" category:

          I've added a patch that fixes this problem. It modifies t_c_glib_generator::generate_serialize_container so variables representing hash-table keys and values are named with a unique suffix, eliminating the possibility of a collision. (This is the same solution used in generate_const_initializer.)

          No specific test case, but the server half of the integration test suite for c_glib will trigger this bug without the patch applied.

          With this fix, the relevant parts of the code generated for Cassandra 1.1.1 now look like this:

          GPtrArray * val75 = NULL;
          g_hash_table_foreach ((GHashTable *)  val73, thrift_hash_table_get_keys, &key_list);
          
          ...
          
          val75 = (GPtrArray *) g_hash_table_lookup (((GHashTable *)  val73), (gpointer) key74);
          
          Show
          Simon South added a comment - In the "better late than never" category: I've added a patch that fixes this problem. It modifies t_c_glib_generator::generate_serialize_container so variables representing hash-table keys and values are named with a unique suffix, eliminating the possibility of a collision. (This is the same solution used in generate_const_initializer .) No specific test case, but the server half of the integration test suite for c_glib will trigger this bug without the patch applied. With this fix, the relevant parts of the code generated for Cassandra 1.1.1 now look like this: GPtrArray * val75 = NULL; g_hash_table_foreach ((GHashTable *) val73, thrift_hash_table_get_keys, &key_list); ... val75 = (GPtrArray *) g_hash_table_lookup (((GHashTable *) val73), (gpointer) key74);
          Hide
          Roger Meier added a comment -

          Great work, thanks Simon!
          ;-r

          Show
          Roger Meier added a comment - Great work, thanks Simon! ;-r
          Hide
          Hudson added a comment -

          FAILURE: Integrated in Thrift #1270 (See https://builds.apache.org/job/Thrift/1270/)
          THRIFT-1266 generated C code for iterating over nested maps is wrong (roger: rev d62473c3b0fff3f50f5d1f7e9dd6f8bdf91d4a66)

          • compiler/cpp/src/generate/t_c_glib_generator.cc
          Show
          Hudson added a comment - FAILURE: Integrated in Thrift #1270 (See https://builds.apache.org/job/Thrift/1270/ ) THRIFT-1266 generated C code for iterating over nested maps is wrong (roger: rev d62473c3b0fff3f50f5d1f7e9dd6f8bdf91d4a66) compiler/cpp/src/generate/t_c_glib_generator.cc
          Hide
          Hudson added a comment -

          FAILURE: Integrated in Thrift-Test #18 (See https://builds.apache.org/job/Thrift-Test/18/)
          THRIFT-1266 generated C code for iterating over nested maps is wrong (roger: rev d62473c3b0fff3f50f5d1f7e9dd6f8bdf91d4a66)

          • compiler/cpp/src/generate/t_c_glib_generator.cc
          Show
          Hudson added a comment - FAILURE: Integrated in Thrift-Test #18 (See https://builds.apache.org/job/Thrift-Test/18/ ) THRIFT-1266 generated C code for iterating over nested maps is wrong (roger: rev d62473c3b0fff3f50f5d1f7e9dd6f8bdf91d4a66) compiler/cpp/src/generate/t_c_glib_generator.cc
          Hide
          Hudson added a comment -

          SUCCESS: Integrated in Thrift #1331 (See https://builds.apache.org/job/Thrift/1331/)
          THRIFT-1266 generated C code for iterating over nested maps is wrong (roger: rev d62473c3b0fff3f50f5d1f7e9dd6f8bdf91d4a66)

          • compiler/cpp/src/generate/t_c_glib_generator.cc
          Show
          Hudson added a comment - SUCCESS: Integrated in Thrift #1331 (See https://builds.apache.org/job/Thrift/1331/ ) THRIFT-1266 generated C code for iterating over nested maps is wrong (roger: rev d62473c3b0fff3f50f5d1f7e9dd6f8bdf91d4a66) compiler/cpp/src/generate/t_c_glib_generator.cc

            People

            • Assignee:
              Simon South
              Reporter:
              Aleksandrs Saveljevs
            • Votes:
              1 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development