-
Notifications
You must be signed in to change notification settings - Fork 51
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 ADIOS1 String Attributes #269
Conversation
Enable serial ADIOS1 on OSX for CI testing.
src/IO/ADIOS/ADIOS1IOHandler.cpp
Outdated
@@ -1335,7 +1335,8 @@ ADIOS1IOHandlerImpl::readAttribute(Writable* writable, | |||
} | |||
} | |||
|
|||
free(data); | |||
if(data) |
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.
does not change the OSX issue
@C0nsultant from the valgrind issues (annotated inline) and the error message, I think we might actually have a little bug on our ADIOS1 IO backend side, at least for the representation of |
Can you narrow down the conditions under which this triggers? If the former is the case, this might actually be a bug in ADIOS attribute writing, rather than reading openPMD-api/include/openPMD/IO/ADIOS/ADIOS1Auxiliary.hpp Lines 118 to 119 in 265828b
openPMD-api/src/IO/ADIOS/ADIOS1IOHandler.cpp Lines 767 to 768 in 265828b
openPMD-api/src/IO/ADIOS/ADIOS1IOHandler.cpp Lines 841 to 847 in 265828b
Relevant ADIOS manual part:
|
I may have spotted the culprit (see #271). openPMD-api/include/openPMD/auxiliary/Variant.hpp Lines 62 to 72 in 0ccf00b
When used to assign the uptr, all is fine. openPMD-api/src/IO/ADIOS/ADIOS1IOHandler.cpp Lines 841 to 847 in 265828b
But as soon as the assignement completes, the temporary std::string gets destroyed.The .c_str() pointer immediately becomes invalid.
|
@@ -977,7 +977,7 @@ ADIOS1IOHandlerImpl::writeAttribute(Writable* writable, | |||
"", | |||
getBP1DataType(parameters.dtype), | |||
nelems, | |||
values.get()); | |||
values.get()); // Invalid read of size 1 (could be in ADIOS itself?) |
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.
in strlen()
in adios_get_type_size (adios_bp_v1.c:2415)
in adios_common_define_attribute_byvalue (adios_internals.c:1109)
in adios_define_attribute_byvalue (adios.c:442)
in openPMD::ADIOS1IOHandlerImpl::writeAttribute
on string value: serial_fileBased_write%T
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.
we already pass the invalid c-string.
strlen((const char*)values.get())
is an invalid read of size 1
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.
Very likely because of #269 (comment).
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.
ah dang, just found it as well :)
yes, we can not defer/forward the pointer we get there :)
src/IO/ADIOS/ADIOS1IOHandler.cpp
Outdated
@@ -842,7 +842,7 @@ ADIOS1IOHandlerImpl::writeAttribute(Writable* writable, | |||
{ | |||
using uptr = std::unique_ptr< void, std::function< void(void*) > >; | |||
values = uptr(const_cast< char* >(att.get< std::string >().c_str()), | |||
[](void*){ }); | |||
[](void*){ }); // leaks memory |
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.
found it, will replace this part
src/IO/ADIOS/ADIOS1IOHandler.cpp
Outdated
[](void*){ }); | ||
auto const & v = att.get< std::string >(); | ||
values = uptr(new char[v.length() + 1u], | ||
[](void* ca){ delete [] (char*)ca; }); |
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.
There's auxiliary::allocatePtr()
;)
Used invalid pointer to string array before.
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.
With the inline comments removed, this is good to go.
Fun fact: the |
You know that |
Fix broken string attribute writes in ADIOS1: Used invalid pointer to string array before and leaked memory as well.
Also enable serial ADIOS1 on OSX for CI testing.
Background
Valgrind says there are a few memory leaks in
std::string
attribute writing.Also, there seems to be a
SerialIOTest
crash in ADIOS1 attribute reading, probably some un-init bytes? Before crash it reads:which looks like a string Datatype issue in our backend.
Message origin in ADIOS: https://github.com/ornladios/ADIOS/blob/v1.13.1/src/core/adios_internals.c#L1146-L1161
During valgrind runs, don't get confused by some noise added by ornladios/ADIOS#184