Avro
  1. Avro
  2. AVRO-1101

avro-c: reference counting error in avro_schema_record_field_get() and friends

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Won't Fix
    • Affects Version/s: 1.6.3, 1.7.0
    • Fix Version/s: None
    • Component/s: c
    • Labels:
      None

      Description

      There is a couple of reference counting errors in avro-c which can produce crash bugs. Attached example shows this problem.

      1. AVRO-1101.patch
        19 kB
        Pugachev Maxim
      2. example3.c
        2 kB
        Pugachev Maxim

        Activity

        Hide
        Pugachev Maxim added a comment -

        Example.

        Show
        Pugachev Maxim added a comment - Example.
        Hide
        Pugachev Maxim added a comment -

        Patch for this issue.

        Show
        Pugachev Maxim added a comment - Patch for this issue.
        Hide
        Pugachev Maxim added a comment -

        The problem is wider than I think. So, I`ve updated a patch.

        Show
        Pugachev Maxim added a comment - The problem is wider than I think. So, I`ve updated a patch.
        Hide
        Pugachev Maxim added a comment -

        Updated patch.

        Show
        Pugachev Maxim added a comment - Updated patch.
        Hide
        Douglas Creager added a comment -

        It's not well-documented, but I think the intention with those accessor functions is that you borrow the reference from the parent schema. That way if you loop through the fields in a record schema, you don't have to call avro_schema_decref on each field. You've got a valid reference to the record, and the record has references to its fields, so you know the fields will be valid for however long you're working with the record.

        If you want to save a reference to the field somewhere else, though, then it's your responsibility to call avro_schema_incref on the field before you stash it away.

        I think this reference counting schema was modeled after the Jansson library; there's a good overview of the borrowed reference idea in their documentation.

        So based on all of this, I think a better patch is to document that these accessor functions return a borrowed reference.

        Show
        Douglas Creager added a comment - It's not well-documented, but I think the intention with those accessor functions is that you borrow the reference from the parent schema. That way if you loop through the fields in a record schema, you don't have to call avro_schema_decref on each field. You've got a valid reference to the record, and the record has references to its fields, so you know the fields will be valid for however long you're working with the record. If you want to save a reference to the field somewhere else, though, then it's your responsibility to call avro_schema_incref on the field before you stash it away. I think this reference counting schema was modeled after the Jansson library; there's a good overview of the borrowed reference idea in their documentation . So based on all of this, I think a better patch is to document that these accessor functions return a borrowed reference.
        Hide
        Pugachev Maxim added a comment -

        For me an idea that some library functions returns a schema with incremented counter and some are not seems not so obvious. In this scenario I must constantly consult the documentation about these rules and keep in mind internal structure of the library. I think that the perfectly clear solution is the condition that I always get incremented reference counter from any function I called, so I always should decrement it afterwards.

        Show
        Pugachev Maxim added a comment - For me an idea that some library functions returns a schema with incremented counter and some are not seems not so obvious. In this scenario I must constantly consult the documentation about these rules and keep in mind internal structure of the library. I think that the perfectly clear solution is the condition that I always get incremented reference counter from any function I called, so I always should decrement it afterwards.
        Hide
        Pugachev Maxim added a comment -

        Guys, do you have any objections to this issue?

        Show
        Pugachev Maxim added a comment - Guys, do you have any objections to this issue?
        Hide
        Douglas Creager added a comment -

        I still disagree with this patch, but haven't closed it yet because I want to hear from some other C contributors.

        I think that the current solution is better. If you call a function that creates a new schema, you have a reference that you have to decrement. If you call a function that returns a subschema of a schema that already exists, then you don't get a new reference automatically. That lets you write a loop very simply:

        avro_schema_t  rec_schema = /* from somewhere */;
        size_t  count = avro_schema_record_size(rec_schema);
        size_t  i;
        for (i = 0; i < count; i++) {
            avro_schema_t  field_schema =
                avro_schema_record_get_by_index(rec_schema, i);
            printf("%zu: %s\n", i, avro_schema_type_name(field_schema));
            /* don't have to decref field_schema */
        }
        

        That missing decref isn't much in this simple example, but internally there are a couple of places where we have to iterate through all of the subschemas when we process a value — avro_value_copy and avro_value_write, for instance. If you call avro_value_write on millions of values, and your schema has 10-20 fields, that's just saved you 10-20 million (atomic) increments and decrements. Those can add up.

        Show
        Douglas Creager added a comment - I still disagree with this patch, but haven't closed it yet because I want to hear from some other C contributors. I think that the current solution is better. If you call a function that creates a new schema, you have a reference that you have to decrement. If you call a function that returns a subschema of a schema that already exists, then you don't get a new reference automatically. That lets you write a loop very simply: avro_schema_t rec_schema = /* from somewhere */; size_t count = avro_schema_record_size(rec_schema); size_t i; for (i = 0; i < count; i++) { avro_schema_t field_schema = avro_schema_record_get_by_index(rec_schema, i); printf( "%zu: %s\n" , i, avro_schema_type_name(field_schema)); /* don't have to decref field_schema */ } That missing decref isn't much in this simple example, but internally there are a couple of places where we have to iterate through all of the subschemas when we process a value — avro_value_copy and avro_value_write, for instance. If you call avro_value_write on millions of values, and your schema has 10-20 fields, that's just saved you 10-20 million (atomic) increments and decrements. Those can add up.
        Hide
        Lucas Martin-King added a comment -

        My colleague (Michael Cooper) and I have contributed some code to the Avro-C libraries.

        We would prefer the current method, (which seems to be more efficient), as we are reading and writing huge amounts of data with Avro. We often encounter segfaults by our own making during testing but these are definitely worth the performance benefits.

        For example, in the case of reading we currently use Avro roughly as follows:

        <create the interface from the reader schema>
        <create a generic value from the interface>
        <load the field indexes from the generic value>
        <decref the generic value>

        Then for each Avro record in a data file:

        <read the record value>
        <read each field by index>
        <reset the value>

        And in the case of writing:

        <create the avro schema from json>
        <open file writer with schema and codec>
        <create the interface from the schema>
        <load the field indexes>
        <create a value "prototype" - we set a lot of fields to null>

        Then writing lots of records:

        <create a generic value from the writer schema>
        <copy the value prototype, ie: avro_value_copy_fast>
        <fill in the values which we need to set>
        <append the value to the writer>
        <decref>

        What we would like to see is some more in depth documentation as we currently use the tests as our reference

        Show
        Lucas Martin-King added a comment - My colleague (Michael Cooper) and I have contributed some code to the Avro-C libraries. We would prefer the current method, (which seems to be more efficient), as we are reading and writing huge amounts of data with Avro. We often encounter segfaults by our own making during testing but these are definitely worth the performance benefits. For example, in the case of reading we currently use Avro roughly as follows: <create the interface from the reader schema> <create a generic value from the interface> <load the field indexes from the generic value> <decref the generic value> Then for each Avro record in a data file: <read the record value> <read each field by index> <reset the value> And in the case of writing: <create the avro schema from json> <open file writer with schema and codec> <create the interface from the schema> <load the field indexes> <create a value "prototype" - we set a lot of fields to null> Then writing lots of records: <create a generic value from the writer schema> <copy the value prototype, ie: avro_value_copy_fast> <fill in the values which we need to set> <append the value to the writer> <decref> What we would like to see is some more in depth documentation as we currently use the tests as our reference
        Hide
        Douglas Creager added a comment -

        Keeping the existing approach for the efficiency reasons described above.

        Show
        Douglas Creager added a comment - Keeping the existing approach for the efficiency reasons described above.

          People

          • Assignee:
            Unassigned
            Reporter:
            Pugachev Maxim
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development