Avro
  1. Avro
  2. AVRO-1033

Avro-C reference count fails on x86 and x86-64 for GCC 4.1.0 and earlier

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.6.2
    • Fix Version/s: 1.6.3
    • Component/s: c
    • Labels:
      None
    • Environment:

      Linux on an x86 or x86-64 platform with GCC version less than or equal to 4.1.0.

      Description

      Item 1:
      *******

      In the Avro-C implementation, there is a bug in the Raw x86 assembly
      performing the atomic increment and decrement operations in
      refcount.h. Specifically, the assembly language commands (mistakenly)
      operate on the the pointer (refcount) instead of operating on the
      memory pointed to (*refcount). This results in memory leaks, when
      trying to deallocate avro values.

      This problem only occurs when using GCC versions less than 4.1.0. For
      GCC versions greater than 4.1.0, the code uses the GCC intrinsic
      __sync_add_and_fetch() or __sync_sub_and_fetch() instead of the raw
      x86 assembly, and this works properly.

      Since this problem is masked for developers working with new(er) GCC
      compilers, greater than version 4.1.0, one way to "uncover" it is to
      force the GCC intrinsics off by requiring a higher GCC version (say
      8.1.0) to use the GCC intrinsics. Then GCC falls back to using the x86
      assembly, and the problem can be seen.

      I am attaching a sample test case (test_refcount.c), which will show a
      memory leak when compiled with the raw x86 commands enabled in
      refcount.h and run with valgrind using the following command:
      valgrind -v --leak-check=full --track-origins=yes ./test_refcount

      I am also attaching a simple patch to fix the bug. The patch just
      replaces (refcount) with (*refcount) in a few places, and the memory
      leak disappears.

      Item 2:
      *******

      On a related note, while compiling Avro across several flavors of
      Linux, we noticed that the GCC atomic intrinsics
      __sync_add_and_fetch() and __sync_sub_and_fetch() were not implemented
      for all versions of GCC greater than 4.1.0, causing compile errors.

      Specifically, the following two versions of GCC don't appear to
      support the GCC atomic intrinsics:
      1. GCC 4.1.2 20080704 (Red Hat 4.1.2-48)
      2. GCC 4.4.3 on Mandriva for an i586 target

      In our local code we have updated the GCC version test in refcount.h
      to require versions of GCC greater than 4.5.0, before including the
      GCC intrinsics. We might want to do the same in the global Avro
      repository. If appropriate, I can create a separate JIRA item for
      this issue. Please let me know.

      Cheers,
      Vivek Nadkarni

      1. AVRO-1033.patch
        0.7 kB
        Vivek Nadkarni
      2. test_refcount.c
        1 kB
        Vivek Nadkarni

        Activity

        Hide
        Vivek Nadkarni added a comment -

        Attach patch - AVRO-1033.patch.
        Attach test program - test_refcount.c.

        Note that you have to force the usage of x86 assembly instead of GCC intrinsics in refcount.h in order to see the memory leak in the test program via valgrind. You can force this usage by modifying refcount.h or by using an older GCC compiler (version <= 4.1.0).

        Show
        Vivek Nadkarni added a comment - Attach patch - AVRO-1033 .patch. Attach test program - test_refcount.c. Note that you have to force the usage of x86 assembly instead of GCC intrinsics in refcount.h in order to see the memory leak in the test program via valgrind. You can force this usage by modifying refcount.h or by using an older GCC compiler (version <= 4.1.0).
        Hide
        Vivek Nadkarni added a comment -

        Attached patch AVRO-1033.patch replaces the pointer (refcount) with the pointed-to value (*refcount) in the assembly language instructions in refcount.h. This allows the assembly instructions to increment and decrement the pointed-to value instead of incrementing and decrementing the pointer.

        Show
        Vivek Nadkarni added a comment - Attached patch AVRO-1033 .patch replaces the pointer (refcount) with the pointed-to value (*refcount) in the assembly language instructions in refcount.h. This allows the assembly instructions to increment and decrement the pointed-to value instead of incrementing and decrementing the pointer.
        Hide
        Douglas Creager added a comment -

        Committed to SVN. I think a new JIRA ticket for the Item 2 is probably the best way to go. Another option, besides bumping the required GCC version in the preprocessor code, would be to perform an explicit autotools-style check in the CMake build scripts to see whether the current compiler supports the GCC intrinsics.

        Show
        Douglas Creager added a comment - Committed to SVN. I think a new JIRA ticket for the Item 2 is probably the best way to go. Another option, besides bumping the required GCC version in the preprocessor code, would be to perform an explicit autotools-style check in the CMake build scripts to see whether the current compiler supports the GCC intrinsics.

          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