-
-
Notifications
You must be signed in to change notification settings - Fork 265
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix ASAN issue in h5dump error path #1051
Conversation
h5dump Tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a comment
tools/src/h5dump/h5dump.c
Outdated
/* table_list.nused will be zero and the added containers needs to be cleaned up */ | ||
HDfree(table_list.tables[0].group_table); | ||
HDfree(table_list.tables[0].dset_table); | ||
HDfree(table_list.tables[0].type_table); | ||
goto done; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not very familiar with the code, but, generally, I think freeing should be done in done:, also, should table_list_free() be used somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally, yes, but these are pointers not captured anywhere else until later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume that since the sanitizer reported an issue, table_list_free() in done wasn't freeing these group, dset, and type tables belonging to table_list.tables[0]? Should table_list_free() in done (line 1614) have freed them? Is there a need to check that they exist before freeing them here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. If these lines stay there, the pointers need to be checked before freeing.
…1070) * Update so numbers for libhdf5_java.so due to functions added to Java API. Remove -avoid-version flag that supresses so version numbers from java/src/jni/Makefile.am. * 1.10 Merge ASAN fixes (#1060) * Merge ASAN fixes Fix ASAN issue in h5dump error path #1051 ASAN fix for test_ld - free sub-allocation of fields #10 * Merge Rework error allocation free. Co-authored-by: Allen Byrne <[email protected]>
Dynamic Analysis indicated an error when there was an error in the table add function, created tables need to be freed in the error block because otherwise there is no indication they were created. The count is only updated after there is an addition to the tables.
==1031476==ERROR: LeakSanitizer: detected memory leaks
Direct leak Direct leak of 32 byte(s) in 1 object(s) allocated from:
#0 0x498bdd in malloc (/home/hdf-winadmin/autotest/hdf5trunk-StdClang-code-ubuntu2010/build/ctest/clang/Address/bin/h5dump-shared+0x498bdd)
#1 0x7f2dad24b2c5 in init_objs (/home/hdf-winadmin/autotest/hdf5trunk-StdClang-code-ubuntu2010/build/ctest/clang/Address/bin/libhdf5_tools.so.1000+0x912c5)
#2 0x4c897f in table_list_add (/home/hdf-winadmin/autotest/hdf5trunk-StdClang-code-ubuntu2010/build/ctest/clang/Address/bin/h5dump-shared+0x4c897f)
#3 0x4ca790 in main (/home/hdf-winadmin/autotest/hdf5trunk-StdClang-code-ubuntu2010/build/ctest/clang/Address/bin/h5dump-shared+0x4ca790)
#4 0x7f2dac025cb1 in __libc_start_main csu/../csu/libc-start.c:314:16