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

Fix access of first element of an empty vector in pynestkernel #2162

Merged
merged 3 commits into from
Sep 14, 2021

Conversation

niltonlk
Copy link
Contributor

This PR fix out of bound access in pynestkernel, reported in #2101.
With this fix, the two pynest examples (recording_demo.py and balancedneuron.py) reported in #2101 now succeed.

@jougs jougs self-assigned this Sep 13, 2021
@jougs jougs added I: Internal API Changes were introduced in basic internal workings of the simulator that developers need to know S: High Should be handled next T: Bug Wrong statements in the code or documentation labels Sep 13, 2021
@jougs jougs added this to the NEST 3.1 milestone Sep 13, 2021
Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

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

LGTM. I suggest a quick merge as this solves a real problem.

Copy link
Contributor

@hakonsbm hakonsbm left a comment

Choose a reason for hiding this comment

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

Just a tiny typo.

pynest/pynestkernel.pyx Outdated Show resolved Hide resolved
@heplesser
Copy link
Contributor

@niltonlk The failure is curious and likely related to using multiple test runners (if several tests write to files of the same name, we will get into trouble). Could you for now change --numprocesses=auto to --numprocesses=1 in these two places:

PYTEST_VERSION="$(${PYTHON} -m pytest --version --timeout ${TIME_LIMIT} --numprocesses=auto 2>&1)" || {

"${PYTHON}" -m pytest --verbose --timeout $TIME_LIMIT --junit-xml="${XUNIT_FILE}" --numprocesses=auto \

following request by @heplesser as a possible cause for the failure in
one of the matrix jobs (several tests write to file of the same name)
@heplesser
Copy link
Contributor

I had another look at the code and am certain that nothing went wrong in the past, because for an empty vector_ptr

memcpy(array_data, &vector_ptr.front(), vector_ptr.size() * sizeof(vector_value_t));

would not have copied anything. But it is clearly incorrect to request front() of an empty container, to this fix is needed.

Copy link
Contributor

@hakonsbm hakonsbm left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: Internal API Changes were introduced in basic internal workings of the simulator that developers need to know S: High Should be handled next T: Bug Wrong statements in the code or documentation
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants