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

Segmentation fault if `Install` is called after `CopyModel` #746

Closed
kappeld opened this Issue Jun 8, 2017 · 1 comment

Comments

Projects
None yet
2 participants
@kappeld
Contributor

kappeld commented Jun 8, 2017

@mhoff and I discovered a strange bug that seems to manifests whenever a custom module is loaded after a call to CopyModel. More precisely the following python code fails with a segmentation fault:

import nest
nest.CopyModel('stdp_synapse','my_synapse')
nest.Install('mymodule')

where mymodule is just the example custom module that comes with NEST.

We tested with the latest commit of NEST using ipython3on Debian Jessie.

I briefly looked into ModelManager::register_connection_model_(...). It seems to me that the syn_id that is assigned to the new synapse models get messed up if the commands are called in that order.

It might make sense to prevent users from calling the commands in that order, but then the function should at least throw an exception in my opinion.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Jun 9, 2017

Contributor

@kappeld @mhoff Thank you for reporting this and for your detective work! I believe the problem is due to the following bit of code

ModelManager::register_connection_model_( ConnectorModel* cf )
{
  ...
  pristine_prototypes_.push_back( cf );
  const synindex syn_id = prototypes_[ 0 ].size();
  pristine_prototypes_[ syn_id ]->set_syn_id( syn_id );

pristine_prototypes_ keeps unmodified copies of the original model prototypes, so that ResetKernel can restore prototypes_. CopyModel only adds new models to prototypes_, not to pristine_prototypes_. If models have been created with CopyModel, prototypes_ will be longer than pristine_prototypes_ and the syn_id will be indexing a non-existing entry in pristine_prototypes_.

I'd suggest the following solution:

  1. Use protected vector access in the last line above, i.e., pristine_prototypes_.at( syn_id )->set_syn_id( syn_id );
  2. Write a regression test based on your example above. With the at() in place, it will reliably break.
  3. Modify LoadModuleFunction to prohibit loading of modules except at the beginning of a "session", i.e., before any models have been copied, neurons created, simulations run; this is a bit overly strict, but it is safe and will not inconvenience users unduly.

Other solutions would be far more complicated. One could consider the following modification of the code above:

  pristine_prototypes_.push_back( cf );
  const synindex syn_id = prototypes_[ 0 ].size();
  // pristine_prototypes_[ syn_id ]->set_syn_id( syn_id );
  pristine_prototypes_.rbegin()->set_syn_id( syn_id );

This would correctly set syn_id on the last entry of pristine_prototypes_, but if a CopyModel has been executed before installing the new module, we will end up with non-contiguous synapse ids in pristine_prototypes_. When we then regenerate prototypes_ during the next ResetKernel, we would be in a lot of trouble, because NEST in many places assumes that syn_ids form a contiguous range.

Contributor

heplesser commented Jun 9, 2017

@kappeld @mhoff Thank you for reporting this and for your detective work! I believe the problem is due to the following bit of code

ModelManager::register_connection_model_( ConnectorModel* cf )
{
  ...
  pristine_prototypes_.push_back( cf );
  const synindex syn_id = prototypes_[ 0 ].size();
  pristine_prototypes_[ syn_id ]->set_syn_id( syn_id );

pristine_prototypes_ keeps unmodified copies of the original model prototypes, so that ResetKernel can restore prototypes_. CopyModel only adds new models to prototypes_, not to pristine_prototypes_. If models have been created with CopyModel, prototypes_ will be longer than pristine_prototypes_ and the syn_id will be indexing a non-existing entry in pristine_prototypes_.

I'd suggest the following solution:

  1. Use protected vector access in the last line above, i.e., pristine_prototypes_.at( syn_id )->set_syn_id( syn_id );
  2. Write a regression test based on your example above. With the at() in place, it will reliably break.
  3. Modify LoadModuleFunction to prohibit loading of modules except at the beginning of a "session", i.e., before any models have been copied, neurons created, simulations run; this is a bit overly strict, but it is safe and will not inconvenience users unduly.

Other solutions would be far more complicated. One could consider the following modification of the code above:

  pristine_prototypes_.push_back( cf );
  const synindex syn_id = prototypes_[ 0 ].size();
  // pristine_prototypes_[ syn_id ]->set_syn_id( syn_id );
  pristine_prototypes_.rbegin()->set_syn_id( syn_id );

This would correctly set syn_id on the last entry of pristine_prototypes_, but if a CopyModel has been executed before installing the new module, we will end up with non-contiguous synapse ids in pristine_prototypes_. When we then regenerate prototypes_ during the next ResetKernel, we would be in a lot of trouble, because NEST in many places assumes that syn_ids form a contiguous range.

@heplesser heplesser self-assigned this Jun 29, 2017

@heplesser heplesser added this to the NEST 2.12.1 milestone Jun 29, 2017

@jougs jougs closed this in #804 Aug 7, 2017

jougs added a commit that referenced this issue Aug 7, 2017

Merge pull request #804 from heplesser/fix-746-module-loading
Prohibit module installation after CopyModel (fixes #746)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment