Skip to content
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

AvalonTools: Avoid that trailing garbage pollutes the fmemopen buffer #5928

Merged
merged 1 commit into from
Jan 7, 2023

Conversation

ptosco
Copy link
Contributor

@ptosco ptosco commented Jan 4, 2023

I found another issue connected with the usage of fmemopen() in Avalon which causes a number of tests involving AvalonTools to fail.
What happens is that, since almost always the buffer used by fmemopen() is larger than the actual bytes written, some trailing garbage is left after the expected content, because the fgets while loop only looks for EOF, and EOF is only met when the end of the buffer is hit (which will always come after the end of the actual content).
The solution is to write a ‘\0’ character after the expected content, and in the fgets while loop also check for zero strlen in addition to checking for EOF.
I have already submitted a patch to Bernd, but until this is fixed upstream we'd better fix it in the RDKit build.

@@ -36,6 +36,16 @@ if(needDownload)
WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR})
endif()

set(reaccsio_c "${AVALONTOOLS_DIR}/src/main/C/common/reaccsio.c")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really a better way of solving this than using a traditional patch?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@greglandrum No, it isn't. But it is cross-platform as it is supported by CMake, while patch is not available on Windows and it is not supported by CMake. I also hope this is only a temporary solution until a new version of Avalon becomes available upstream.

@greglandrum greglandrum changed the title Avoid that trailing garbage pollutes the fmemopen buffer AvalonTools: Avoid that trailing garbage pollutes the fmemopen buffer Jan 7, 2023
@greglandrum greglandrum added the bug label Jan 7, 2023
@greglandrum greglandrum added this to the 2022_09_4 milestone Jan 7, 2023
@greglandrum greglandrum merged commit 206b698 into rdkit:master Jan 7, 2023
@greglandrum
Copy link
Member

@ptosco I just noticed that this runs every time you run cmake. So we end up with multiple definitions of Len in reaccsio.c
Do you want to fix that or shall I?

@ptosco
Copy link
Contributor Author

ptosco commented Jan 10, 2023

@greglandrum Yes, I noticed that yesterday and already submitted a PR to fix it: #5951.

@ptosco ptosco deleted the avalon_fmemopen_patch branch January 10, 2023 09:28
@greglandrum
Copy link
Member

@greglandrum Yes, I noticed that yesterday and already submitted a PR to fix it: #5951.

ah, awesome. Thanks.
I obviously haven't looked at new PRs yet.

greglandrum pushed a commit that referenced this pull request Feb 23, 2023
Co-authored-by: Tosco, Paolo <paolo.tosco@novartis.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants