You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by "Vivek Nadkarni (Created) (JIRA)" <ji...@apache.org> on 2011/10/16 01:50:11 UTC
[jira] [Created] (AVRO-930) Memory leak in resolved-writer.c in
Avro C
Memory leak in resolved-writer.c in Avro C
------------------------------------------
Key: AVRO-930
URL: https://issues.apache.org/jira/browse/AVRO-930
Project: Avro
Issue Type: Bug
Components: c
Affects Versions: 1.6.0
Environment: All.
Reporter: Vivek Nadkarni
Priority: Minor
Fix For: 1.6.0
I was able to isolate a memory leak in the new schema resolver
(resolved-writer.c) in Avro C on the avro-trunk (v1.6.0), in which the
code attempts to access and free memory that has already been freed.
There is a simple one-line fix to this issue, using the reference
counting mechanism provided in Avro, attached in the patch file.
This patch is different from the one I originally sent to the Avro Dev
mailing list, and I think my new patch is better.
Longer Description:
I created a small test program to test schema resolution in C. The
program contains a writer schema with two integer elements and a reader
schema with three integer elements. Since the reader schema has elements
that the writer doesn't contain, and default values are not yet
supported in C, the function try_record() goes into the "error" section
(by design) and tries to clean up after itself. This cleanup process has
a memory bug in it.
Specifically, if the reader schema contains two elements of the same
type (in my case two ints), try_record() uses the same resolver for
both elements. This common resolver is used because the function
avro_resolved_writer_new_memoized() tests to see if the schema's of
the two elements are the same, and if they are matched, it returns the
resolver, and does not create a new resolver. It also does not
increment the reference count of this existing resolver. So, multiple
elements of the array of field_resolvers[] can contain pointers to the
same resolver, and these reference are not accounted for in the
reference count.
During the cleanup process, there is a loop that checks if the pointer
field_resolvers[i] is non-NULL, and tries to free all non-NULL
resolvers. Since we have two (or more) elements of the
field_resolvers[] array pointing to the same resolver, and the
reference count of the resolver doesn't count these additional
references, the code tries to call avro_value_iface_decref() on an
already freed resolver.
This can be simply fixed as follows: Increment the reference count
each time you assign a resolver to the field_resolvers[] array, even
when you are re-assigning the same resolver to a new element in the
array.
I verified this patch using valgrind. Multiple valgrind errors were
seen in try_record() and avro_refcount_dec() before applying the
patch, and these errors went away after applying the patch.
In an earlier email to the avro dev mailing list, I provided a patch
which didn't use the reference count mechanism. Instead it checked the
field_resolvers[] array for duplicate entries, and set the duplicates
to NULL. This patch used more code, but had a more localized impact --
in that it was exercised only when an error condition was
encountered. I can provide this patch if anyone wants it.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (AVRO-930) Memory leak in resolved-writer.c in
Avro C
Posted by "Doug Cutting (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/AVRO-930?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Doug Cutting updated AVRO-930:
------------------------------
Priority: Blocker (was: Minor)
Upgrading to blocker as we prepare for 1.6.0.
> Memory leak in resolved-writer.c in Avro C
> ------------------------------------------
>
> Key: AVRO-930
> URL: https://issues.apache.org/jira/browse/AVRO-930
> Project: Avro
> Issue Type: Bug
> Components: c
> Affects Versions: 1.6.0
> Environment: All.
> Reporter: Vivek Nadkarni
> Priority: Blocker
> Fix For: 1.6.0
>
> Attachments: AVRO-930.patch, avro-930-test.c
>
> Original Estimate: 24h
> Remaining Estimate: 24h
>
> I was able to isolate a memory leak in the new schema resolver
> (resolved-writer.c) in Avro C on the avro-trunk (v1.6.0), in which the
> code attempts to access and free memory that has already been freed.
> There is a simple one-line fix to this issue, using the reference
> counting mechanism provided in Avro, attached in the patch file.
> This patch is different from the one I originally sent to the Avro Dev
> mailing list, and I think my new patch is better.
> Longer Description:
> I created a small test program to test schema resolution in C. The
> program contains a writer schema with two integer elements and a reader
> schema with three integer elements. Since the reader schema has elements
> that the writer doesn't contain, and default values are not yet
> supported in C, the function try_record() goes into the "error" section
> (by design) and tries to clean up after itself. This cleanup process has
> a memory bug in it.
> Specifically, if the reader schema contains two elements of the same
> type (in my case two ints), try_record() uses the same resolver for
> both elements. This common resolver is used because the function
> avro_resolved_writer_new_memoized() tests to see if the schema's of
> the two elements are the same, and if they are matched, it returns the
> resolver, and does not create a new resolver. It also does not
> increment the reference count of this existing resolver. So, multiple
> elements of the array of field_resolvers[] can contain pointers to the
> same resolver, and these reference are not accounted for in the
> reference count.
> During the cleanup process, there is a loop that checks if the pointer
> field_resolvers[i] is non-NULL, and tries to free all non-NULL
> resolvers. Since we have two (or more) elements of the
> field_resolvers[] array pointing to the same resolver, and the
> reference count of the resolver doesn't count these additional
> references, the code tries to call avro_value_iface_decref() on an
> already freed resolver.
> This can be simply fixed as follows: Increment the reference count
> each time you assign a resolver to the field_resolvers[] array, even
> when you are re-assigning the same resolver to a new element in the
> array.
> I verified this patch using valgrind. Multiple valgrind errors were
> seen in try_record() and avro_refcount_dec() before applying the
> patch, and these errors went away after applying the patch.
> In an earlier email to the avro dev mailing list, I provided a patch
> which didn't use the reference count mechanism. Instead it checked the
> field_resolvers[] array for duplicate entries, and set the duplicates
> to NULL. This patch used more code, but had a more localized impact --
> in that it was exercised only when an error condition was
> encountered. I can provide this patch if anyone wants it.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (AVRO-930) Memory leak in resolved-writer.c in
Avro C
Posted by "Vivek Nadkarni (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/AVRO-930?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Vivek Nadkarni updated AVRO-930:
--------------------------------
Attachment: avro-930-test.c
Test code for JIRA Issue AVRO-930.
This program tests the error handling that arises when the reader
schema contains fields that the writer schema has not written. The
Avro Spec allows the reader to use default values for each field. The
Avro C code does not currently support default values -- this is
listed as a TODO in the function try_record() in the file
resolved-writer.c.
This file was stored in a temporary directory under
avro-trunk/lang/c/avro-930-test/avro-930-test.c
It was compiled under Linux using:
gcc -g avro-930-test.c -o avro930 -I../src -L../src -lavro
The code was tested with valgrind using the command:
valgrind -v --leak-check=full ./avro930
Before the patch for AVRO-930, valgrind shows multiple errors. After
the patch, no valgrind errors are seen.
> Memory leak in resolved-writer.c in Avro C
> ------------------------------------------
>
> Key: AVRO-930
> URL: https://issues.apache.org/jira/browse/AVRO-930
> Project: Avro
> Issue Type: Bug
> Components: c
> Affects Versions: 1.6.0
> Environment: All.
> Reporter: Vivek Nadkarni
> Priority: Minor
> Fix For: 1.6.0
>
> Attachments: AVRO-930.patch, avro-930-test.c
>
> Original Estimate: 24h
> Remaining Estimate: 24h
>
> I was able to isolate a memory leak in the new schema resolver
> (resolved-writer.c) in Avro C on the avro-trunk (v1.6.0), in which the
> code attempts to access and free memory that has already been freed.
> There is a simple one-line fix to this issue, using the reference
> counting mechanism provided in Avro, attached in the patch file.
> This patch is different from the one I originally sent to the Avro Dev
> mailing list, and I think my new patch is better.
> Longer Description:
> I created a small test program to test schema resolution in C. The
> program contains a writer schema with two integer elements and a reader
> schema with three integer elements. Since the reader schema has elements
> that the writer doesn't contain, and default values are not yet
> supported in C, the function try_record() goes into the "error" section
> (by design) and tries to clean up after itself. This cleanup process has
> a memory bug in it.
> Specifically, if the reader schema contains two elements of the same
> type (in my case two ints), try_record() uses the same resolver for
> both elements. This common resolver is used because the function
> avro_resolved_writer_new_memoized() tests to see if the schema's of
> the two elements are the same, and if they are matched, it returns the
> resolver, and does not create a new resolver. It also does not
> increment the reference count of this existing resolver. So, multiple
> elements of the array of field_resolvers[] can contain pointers to the
> same resolver, and these reference are not accounted for in the
> reference count.
> During the cleanup process, there is a loop that checks if the pointer
> field_resolvers[i] is non-NULL, and tries to free all non-NULL
> resolvers. Since we have two (or more) elements of the
> field_resolvers[] array pointing to the same resolver, and the
> reference count of the resolver doesn't count these additional
> references, the code tries to call avro_value_iface_decref() on an
> already freed resolver.
> This can be simply fixed as follows: Increment the reference count
> each time you assign a resolver to the field_resolvers[] array, even
> when you are re-assigning the same resolver to a new element in the
> array.
> I verified this patch using valgrind. Multiple valgrind errors were
> seen in try_record() and avro_refcount_dec() before applying the
> patch, and these errors went away after applying the patch.
> In an earlier email to the avro dev mailing list, I provided a patch
> which didn't use the reference count mechanism. Instead it checked the
> field_resolvers[] array for duplicate entries, and set the duplicates
> to NULL. This patch used more code, but had a more localized impact --
> in that it was exercised only when an error condition was
> encountered. I can provide this patch if anyone wants it.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (AVRO-930) Memory leak in resolved-writer.c in
Avro C
Posted by "Vivek Nadkarni (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/AVRO-930?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Vivek Nadkarni updated AVRO-930:
--------------------------------
Status: Patch Available (was: Open)
The patch I attached is ready for review. Please let me know if you need any changes.
> Memory leak in resolved-writer.c in Avro C
> ------------------------------------------
>
> Key: AVRO-930
> URL: https://issues.apache.org/jira/browse/AVRO-930
> Project: Avro
> Issue Type: Bug
> Components: c
> Affects Versions: 1.6.0
> Environment: All.
> Reporter: Vivek Nadkarni
> Priority: Minor
> Fix For: 1.6.0
>
> Attachments: AVRO-930.patch
>
> Original Estimate: 24h
> Remaining Estimate: 24h
>
> I was able to isolate a memory leak in the new schema resolver
> (resolved-writer.c) in Avro C on the avro-trunk (v1.6.0), in which the
> code attempts to access and free memory that has already been freed.
> There is a simple one-line fix to this issue, using the reference
> counting mechanism provided in Avro, attached in the patch file.
> This patch is different from the one I originally sent to the Avro Dev
> mailing list, and I think my new patch is better.
> Longer Description:
> I created a small test program to test schema resolution in C. The
> program contains a writer schema with two integer elements and a reader
> schema with three integer elements. Since the reader schema has elements
> that the writer doesn't contain, and default values are not yet
> supported in C, the function try_record() goes into the "error" section
> (by design) and tries to clean up after itself. This cleanup process has
> a memory bug in it.
> Specifically, if the reader schema contains two elements of the same
> type (in my case two ints), try_record() uses the same resolver for
> both elements. This common resolver is used because the function
> avro_resolved_writer_new_memoized() tests to see if the schema's of
> the two elements are the same, and if they are matched, it returns the
> resolver, and does not create a new resolver. It also does not
> increment the reference count of this existing resolver. So, multiple
> elements of the array of field_resolvers[] can contain pointers to the
> same resolver, and these reference are not accounted for in the
> reference count.
> During the cleanup process, there is a loop that checks if the pointer
> field_resolvers[i] is non-NULL, and tries to free all non-NULL
> resolvers. Since we have two (or more) elements of the
> field_resolvers[] array pointing to the same resolver, and the
> reference count of the resolver doesn't count these additional
> references, the code tries to call avro_value_iface_decref() on an
> already freed resolver.
> This can be simply fixed as follows: Increment the reference count
> each time you assign a resolver to the field_resolvers[] array, even
> when you are re-assigning the same resolver to a new element in the
> array.
> I verified this patch using valgrind. Multiple valgrind errors were
> seen in try_record() and avro_refcount_dec() before applying the
> patch, and these errors went away after applying the patch.
> In an earlier email to the avro dev mailing list, I provided a patch
> which didn't use the reference count mechanism. Instead it checked the
> field_resolvers[] array for duplicate entries, and set the duplicates
> to NULL. This patch used more code, but had a more localized impact --
> in that it was exercised only when an error condition was
> encountered. I can provide this patch if anyone wants it.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (AVRO-930) Memory leak in resolved-writer.c in
Avro C
Posted by "Vivek Nadkarni (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/AVRO-930?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Vivek Nadkarni updated AVRO-930:
--------------------------------
Attachment: AVRO-930.patch
Here is the one line patch that increments the ref-count when an existing resolver is assigned to the resolvers[] array.
> Memory leak in resolved-writer.c in Avro C
> ------------------------------------------
>
> Key: AVRO-930
> URL: https://issues.apache.org/jira/browse/AVRO-930
> Project: Avro
> Issue Type: Bug
> Components: c
> Affects Versions: 1.6.0
> Environment: All.
> Reporter: Vivek Nadkarni
> Priority: Minor
> Fix For: 1.6.0
>
> Attachments: AVRO-930.patch
>
> Original Estimate: 24h
> Remaining Estimate: 24h
>
> I was able to isolate a memory leak in the new schema resolver
> (resolved-writer.c) in Avro C on the avro-trunk (v1.6.0), in which the
> code attempts to access and free memory that has already been freed.
> There is a simple one-line fix to this issue, using the reference
> counting mechanism provided in Avro, attached in the patch file.
> This patch is different from the one I originally sent to the Avro Dev
> mailing list, and I think my new patch is better.
> Longer Description:
> I created a small test program to test schema resolution in C. The
> program contains a writer schema with two integer elements and a reader
> schema with three integer elements. Since the reader schema has elements
> that the writer doesn't contain, and default values are not yet
> supported in C, the function try_record() goes into the "error" section
> (by design) and tries to clean up after itself. This cleanup process has
> a memory bug in it.
> Specifically, if the reader schema contains two elements of the same
> type (in my case two ints), try_record() uses the same resolver for
> both elements. This common resolver is used because the function
> avro_resolved_writer_new_memoized() tests to see if the schema's of
> the two elements are the same, and if they are matched, it returns the
> resolver, and does not create a new resolver. It also does not
> increment the reference count of this existing resolver. So, multiple
> elements of the array of field_resolvers[] can contain pointers to the
> same resolver, and these reference are not accounted for in the
> reference count.
> During the cleanup process, there is a loop that checks if the pointer
> field_resolvers[i] is non-NULL, and tries to free all non-NULL
> resolvers. Since we have two (or more) elements of the
> field_resolvers[] array pointing to the same resolver, and the
> reference count of the resolver doesn't count these additional
> references, the code tries to call avro_value_iface_decref() on an
> already freed resolver.
> This can be simply fixed as follows: Increment the reference count
> each time you assign a resolver to the field_resolvers[] array, even
> when you are re-assigning the same resolver to a new element in the
> array.
> I verified this patch using valgrind. Multiple valgrind errors were
> seen in try_record() and avro_refcount_dec() before applying the
> patch, and these errors went away after applying the patch.
> In an earlier email to the avro dev mailing list, I provided a patch
> which didn't use the reference count mechanism. Instead it checked the
> field_resolvers[] array for duplicate entries, and set the duplicates
> to NULL. This patch used more code, but had a more localized impact --
> in that it was exercised only when an error condition was
> encountered. I can provide this patch if anyone wants it.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (AVRO-930) Memory leak in resolved-writer.c in
Avro C
Posted by "Douglas Creager (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/AVRO-930?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Douglas Creager updated AVRO-930:
---------------------------------
Resolution: Fixed
Assignee: Douglas Creager
Status: Resolved (was: Patch Available)
Sorry for the delay in committing this — I was away over the weekend. This patch is in SVN trunk now. Thanks Vivek!
> Memory leak in resolved-writer.c in Avro C
> ------------------------------------------
>
> Key: AVRO-930
> URL: https://issues.apache.org/jira/browse/AVRO-930
> Project: Avro
> Issue Type: Bug
> Components: c
> Affects Versions: 1.6.0
> Environment: All.
> Reporter: Vivek Nadkarni
> Assignee: Douglas Creager
> Priority: Blocker
> Fix For: 1.6.0
>
> Attachments: AVRO-930.patch, avro-930-test.c
>
> Original Estimate: 24h
> Remaining Estimate: 24h
>
> I was able to isolate a memory leak in the new schema resolver
> (resolved-writer.c) in Avro C on the avro-trunk (v1.6.0), in which the
> code attempts to access and free memory that has already been freed.
> There is a simple one-line fix to this issue, using the reference
> counting mechanism provided in Avro, attached in the patch file.
> This patch is different from the one I originally sent to the Avro Dev
> mailing list, and I think my new patch is better.
> Longer Description:
> I created a small test program to test schema resolution in C. The
> program contains a writer schema with two integer elements and a reader
> schema with three integer elements. Since the reader schema has elements
> that the writer doesn't contain, and default values are not yet
> supported in C, the function try_record() goes into the "error" section
> (by design) and tries to clean up after itself. This cleanup process has
> a memory bug in it.
> Specifically, if the reader schema contains two elements of the same
> type (in my case two ints), try_record() uses the same resolver for
> both elements. This common resolver is used because the function
> avro_resolved_writer_new_memoized() tests to see if the schema's of
> the two elements are the same, and if they are matched, it returns the
> resolver, and does not create a new resolver. It also does not
> increment the reference count of this existing resolver. So, multiple
> elements of the array of field_resolvers[] can contain pointers to the
> same resolver, and these reference are not accounted for in the
> reference count.
> During the cleanup process, there is a loop that checks if the pointer
> field_resolvers[i] is non-NULL, and tries to free all non-NULL
> resolvers. Since we have two (or more) elements of the
> field_resolvers[] array pointing to the same resolver, and the
> reference count of the resolver doesn't count these additional
> references, the code tries to call avro_value_iface_decref() on an
> already freed resolver.
> This can be simply fixed as follows: Increment the reference count
> each time you assign a resolver to the field_resolvers[] array, even
> when you are re-assigning the same resolver to a new element in the
> array.
> I verified this patch using valgrind. Multiple valgrind errors were
> seen in try_record() and avro_refcount_dec() before applying the
> patch, and these errors went away after applying the patch.
> In an earlier email to the avro dev mailing list, I provided a patch
> which didn't use the reference count mechanism. Instead it checked the
> field_resolvers[] array for duplicate entries, and set the duplicates
> to NULL. This patch used more code, but had a more localized impact --
> in that it was exercised only when an error condition was
> encountered. I can provide this patch if anyone wants it.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (AVRO-930) Memory leak in resolved-writer.c in
Avro C
Posted by "Vivek Nadkarni (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/AVRO-930?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13129917#comment-13129917 ]
Vivek Nadkarni commented on AVRO-930:
-------------------------------------
I should add, the desired behavior of the test case I just uploaded is to exit with EXIT_FAILURE, but without any memory problems in Valgrind.
> Memory leak in resolved-writer.c in Avro C
> ------------------------------------------
>
> Key: AVRO-930
> URL: https://issues.apache.org/jira/browse/AVRO-930
> Project: Avro
> Issue Type: Bug
> Components: c
> Affects Versions: 1.6.0
> Environment: All.
> Reporter: Vivek Nadkarni
> Priority: Minor
> Fix For: 1.6.0
>
> Attachments: AVRO-930.patch, avro-930-test.c
>
> Original Estimate: 24h
> Remaining Estimate: 24h
>
> I was able to isolate a memory leak in the new schema resolver
> (resolved-writer.c) in Avro C on the avro-trunk (v1.6.0), in which the
> code attempts to access and free memory that has already been freed.
> There is a simple one-line fix to this issue, using the reference
> counting mechanism provided in Avro, attached in the patch file.
> This patch is different from the one I originally sent to the Avro Dev
> mailing list, and I think my new patch is better.
> Longer Description:
> I created a small test program to test schema resolution in C. The
> program contains a writer schema with two integer elements and a reader
> schema with three integer elements. Since the reader schema has elements
> that the writer doesn't contain, and default values are not yet
> supported in C, the function try_record() goes into the "error" section
> (by design) and tries to clean up after itself. This cleanup process has
> a memory bug in it.
> Specifically, if the reader schema contains two elements of the same
> type (in my case two ints), try_record() uses the same resolver for
> both elements. This common resolver is used because the function
> avro_resolved_writer_new_memoized() tests to see if the schema's of
> the two elements are the same, and if they are matched, it returns the
> resolver, and does not create a new resolver. It also does not
> increment the reference count of this existing resolver. So, multiple
> elements of the array of field_resolvers[] can contain pointers to the
> same resolver, and these reference are not accounted for in the
> reference count.
> During the cleanup process, there is a loop that checks if the pointer
> field_resolvers[i] is non-NULL, and tries to free all non-NULL
> resolvers. Since we have two (or more) elements of the
> field_resolvers[] array pointing to the same resolver, and the
> reference count of the resolver doesn't count these additional
> references, the code tries to call avro_value_iface_decref() on an
> already freed resolver.
> This can be simply fixed as follows: Increment the reference count
> each time you assign a resolver to the field_resolvers[] array, even
> when you are re-assigning the same resolver to a new element in the
> array.
> I verified this patch using valgrind. Multiple valgrind errors were
> seen in try_record() and avro_refcount_dec() before applying the
> patch, and these errors went away after applying the patch.
> In an earlier email to the avro dev mailing list, I provided a patch
> which didn't use the reference count mechanism. Instead it checked the
> field_resolvers[] array for duplicate entries, and set the duplicates
> to NULL. This patch used more code, but had a more localized impact --
> in that it was exercised only when an error condition was
> encountered. I can provide this patch if anyone wants it.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira