Avro
  1. Avro
  2. AVRO-980

C: avro_schema_from_json ignores length parameter

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.6.1
    • Fix Version/s: 1.6.2
    • Component/s: c
    • Labels:
    • Environment:

      Linux x86_64 (Ubuntu 11.10)
      Avro 1.6.1
      GCC 4.6.1
      cmake 2.8.5

      Description

      The function avro_schema_from_json() takes in a const char* and a length parameter, but it appears that the length parameter is ignored.

      We have a project where the schema is a binary resource compiled into the executable, and is not guaranteed to have a null terminator at the end.
      This did not appear to be an issue because the function had a length parameter.

      The other day, It kept throwing errors about characters that were not in the file at all.
      Suspecting it was running off the end of the file, I had a look at the Avro-C code and found that even though the function takes in a length parameter, it does not use it and expects a null terminator.

        Activity

        Hide
        Douglas Creager added a comment -

        Committed to SVN

        Show
        Douglas Creager added a comment - Committed to SVN
        Hide
        Douglas Creager added a comment -

        I'd like to get this patch in for the 1.6.2 release. If I don't hear any objections in by tomorrow, I'll commit to SVN.

        Show
        Douglas Creager added a comment - I'd like to get this patch in for the 1.6.2 release. If I don't hear any objections in by tomorrow, I'll commit to SVN.
        Hide
        Douglas Creager added a comment -

        I've put together a better patch for this issue. Here's the commit message from the patch file:

        We now explicitly document that the length and error parameters are unused in the avro_schema_from_json function. (The error parameter isn't needed, since any errors parsing or interpreting the JSON text are available using the avro_strerror() function.)

        This patch also adds the new avro_schema_from_json_length function. This function actually uses its length parameter, and we explicitly document that the length should not include any NUL terminator, if one is present. We also provide the avro_schema_from_json_literal helper macro, which automatically calculates the size of a literal JSON string at compile time. (To work, the JSON string must be defined as a char[], not a char *.)

        We decided to fix this bug using a new function because there's existing code out there that's already assuming that avro_schema_from_json's len parameter is unused. (Or at least, they're assuming different things about what kind of value to pass in.) This solution ensures that existing code still works, while providing a new function for the (needed) non-NUL-terminated case. It comes at the expense of a sloppy signature for the existing avro_schema_from_json function...but then, the signature was already sloppy. We're at least not adding any new sloppiness.

        We also now use the new avro_schema_from_json_length function when reading an Avro object container file. This means that we no longer need to append a NUL terminator to the schema JSON when reading in the container file's header. (We don't need to, but we still do. One change at a time.)

        Show
        Douglas Creager added a comment - I've put together a better patch for this issue. Here's the commit message from the patch file: We now explicitly document that the length and error parameters are unused in the avro_schema_from_json function. (The error parameter isn't needed, since any errors parsing or interpreting the JSON text are available using the avro_strerror() function.) This patch also adds the new avro_schema_from_json_length function. This function actually uses its length parameter, and we explicitly document that the length should not include any NUL terminator, if one is present. We also provide the avro_schema_from_json_literal helper macro, which automatically calculates the size of a literal JSON string at compile time. (To work, the JSON string must be defined as a char[], not a char *.) We decided to fix this bug using a new function because there's existing code out there that's already assuming that avro_schema_from_json's len parameter is unused. (Or at least, they're assuming different things about what kind of value to pass in.) This solution ensures that existing code still works, while providing a new function for the (needed) non-NUL-terminated case. It comes at the expense of a sloppy signature for the existing avro_schema_from_json function...but then, the signature was already sloppy. We're at least not adding any new sloppiness. We also now use the new avro_schema_from_json_length function when reading an Avro object container file. This means that we no longer need to append a NUL terminator to the schema JSON when reading in the container file's header. (We don't need to , but we still do. One change at a time.)
        Hide
        Douglas Creager added a comment -

        In particular, a colleague of mine points out that any code following this example in the documentation will break with this patch, since the sizeof(PERSON_SCHEMA) should be replaced with sizeof(PERSON_SCHEMA)-1 or strlen(PERSON_SCHEMA). (The length shouldn't include the NUL terminator.)

        Show
        Douglas Creager added a comment - In particular, a colleague of mine points out that any code following this example in the documentation will break with this patch, since the sizeof(PERSON_SCHEMA) should be replaced with sizeof(PERSON_SCHEMA)-1 or strlen(PERSON_SCHEMA) . (The length shouldn't include the NUL terminator.)
        Hide
        Douglas Creager added a comment -

        Maybe we can assume a NUL-terminated string if the caller passes in 0 for the length parameter?

        Show
        Douglas Creager added a comment - Maybe we can assume a NUL-terminated string if the caller passes in 0 for the length parameter?
        Hide
        Douglas Creager added a comment -

        Here's a patch for this. Luckily we upgraded our internal copy of Jansson to 2.1 awhile back — it has a json_loadb function, which takes in a length parameter. (Before, we used json_loads, which assumed a NUL-terminated string.)

        One thing we might need to consider is that this might break existing code, if they were passing in incorrect (or dummy) values for the length parameter.

        Show
        Douglas Creager added a comment - Here's a patch for this. Luckily we upgraded our internal copy of Jansson to 2.1 awhile back — it has a json_loadb function, which takes in a length parameter. (Before, we used json_loads, which assumed a NUL-terminated string.) One thing we might need to consider is that this might break existing code, if they were passing in incorrect (or dummy) values for the length parameter.

          People

          • Assignee:
            Unassigned
            Reporter:
            Michael Cooper
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development