Avro
  1. Avro
  2. AVRO-968

Avro C - avro_value_cmp_fast() may return garbage value for AVRO_STRING comparison

    Details

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

      All. Currently using gcc 4.6.1 on Ubuntu 11.10.

    • Release Note:
      Fix uninitialized variable warning in value.c, by initializing the variables properly.

      Description

      Compiler shows a warning that variables may be used uninitialized in avro_value_cmp_fast():

      /home/user/avro-trunk/lang/c/src/value.c: In function 'avro_value_cmp_fast':
      /home/user/avro-trunk/lang/c/src/value.c:387:13: warning: 'size2' may be used uninitialized in this function [-Wuninitialized]
      /home/user/avro-trunk/lang/c/src/value.c:387:13: warning: 'size1' may be used uninitialized in this function [-Wuninitialized]
      /home/user/avro-trunk/lang/c/src/value.c:388:11: warning: 'buf1' may be used uninitialized in this function [-Wuninitialized]
      /home/user/avro-trunk/lang/c/src/value.c:388:11: warning: 'buf2' may be used uninitialized in this function [-Wuninitialized]

      Examining the file shows that the warnings are real, and the variables size1, buf1, size2, buf2 should be loaded before they are used. The simple fix is to copy matching code from the function avro_value_equal_fast(). I will attach that code in an upcoming patch.

      Cheers,
      Vivek

        Activity

        Hide
        Douglas Creager added a comment -

        Yeah, I committed the patch a few weeks ago. Wasn't sure if we needed to discuss the refactoring that you suggested. We can open up another issue for that if we choose to do that. Closing!

        Show
        Douglas Creager added a comment - Yeah, I committed the patch a few weeks ago. Wasn't sure if we needed to discuss the refactoring that you suggested. We can open up another issue for that if we choose to do that. Closing!
        Hide
        Vivek Nadkarni added a comment -

        Yes you can mark this issue resolved as fixed, once the updated patch that Douglas Creager provided is committed to Subversion.

        Show
        Vivek Nadkarni added a comment - Yes you can mark this issue resolved as fixed, once the updated patch that Douglas Creager provided is committed to Subversion.
        Hide
        Doug Cutting added a comment -

        This issue can be resolved as fixed, right?

        Show
        Doug Cutting added a comment - This issue can be resolved as fixed, right?
        Hide
        Douglas Creager added a comment -

        Also, there is a difference between _cmp and _equal. You can't do an arbitrary comparison of map values using _cmp or _cmp_fast, since the spec doesn't define a sort order for them. But you can compare them for equality, by checking that the set of keys is identical, and that each key maps to equal values.

        That said, there's still some refactoring that we could do there.

        Show
        Douglas Creager added a comment - Also, there is a difference between _cmp and _equal. You can't do an arbitrary comparison of map values using _cmp or _cmp_fast, since the spec doesn't define a sort order for them. But you can compare them for equality, by checking that the set of keys is identical, and that each key maps to equal values. That said, there's still some refactoring that we could do there.
        Hide
        Douglas Creager added a comment -

        Here's an updated version that includes a test case.

        Show
        Douglas Creager added a comment - Here's an updated version that includes a test case.
        Hide
        Vivek Nadkarni added a comment -

        On a related note, I see significant duplicated code in avro_value_equal_fast() and avro_value_cmp_fast(). Couldn't avro_value_equal_fast() be rewritten more simply as

        int avro_value_equal_fast( avro_value_t *val1, avro_value_t *val2)

        { return ( avro_value_cmp_fast( val1, val2 ) == 0 ); }

        Thanks,
        Vivek

        Show
        Vivek Nadkarni added a comment - On a related note, I see significant duplicated code in avro_value_equal_fast() and avro_value_cmp_fast(). Couldn't avro_value_equal_fast() be rewritten more simply as int avro_value_equal_fast( avro_value_t *val1, avro_value_t *val2) { return ( avro_value_cmp_fast( val1, val2 ) == 0 ); } Thanks, Vivek
        Hide
        Vivek Nadkarni added a comment -

        This patch fixes the warnings in the compile.

        Show
        Vivek Nadkarni added a comment - This patch fixes the warnings in the compile.
        Hide
        Vivek Nadkarni added a comment -

        Patch to fix AVRO-968.

        Show
        Vivek Nadkarni added a comment - Patch to fix AVRO-968 .

          People

          • Assignee:
            Douglas Creager
            Reporter:
            Vivek Nadkarni
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 24h
              24h
              Remaining:
              Remaining Estimate - 24h
              24h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development