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.

Sign up for GitHub

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 out of bound access on pp_pop_psc_delta and pulsepacket_generator #2159

Merged
merged 6 commits into from
Sep 13, 2021
Merged

Fix out of bound access on pp_pop_psc_delta and pulsepacket_generator #2159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f64c177
Fix out of bound access on pp_pop_psc_delta and pulsepacket_generator
niltonlk Sep 8, 2021
b4d1ae0
Replace || with or, and && with and, following comments by @heplesser
niltonlk Sep 8, 2021
7d8bd63
Fix last spike was not sent due to error in conditional expression.
niltonlk Sep 9, 2021
31a8b99
Add a comment: Set all except last state vector elements to zero, the…
niltonlk Sep 9, 2021
10f3651
Add deprecation message.
niltonlk Sep 13, 2021
205e7c2
Fix formatting
niltonlk Sep 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion models/modelsmodule.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,8 @@ ModelsModule::init( SLIInterpreter* )
kernel().model_manager.register_node_model< parrot_neuron >( "parrot_neuron" );
kernel().model_manager.register_node_model< parrot_neuron_ps >( "parrot_neuron_ps" );
kernel().model_manager.register_node_model< pp_psc_delta >( "pp_psc_delta" );
kernel().model_manager.register_node_model< pp_pop_psc_delta >( "pp_pop_psc_delta" );
kernel().model_manager.register_node_model< pp_pop_psc_delta >(
"pp_pop_psc_delta", /*private_model*/ false, /*deprecation_info*/ "a future version of NEST" );
kernel().model_manager.register_node_model< gif_psc_exp >( "gif_psc_exp" );
kernel().model_manager.register_node_model< gif_psc_exp_multisynapse >( "gif_psc_exp_multisynapse" );
kernel().model_manager.register_node_model< glif_psc >( "glif_psc" );
Expand Down
5 changes: 3 additions & 2 deletions models/pp_pop_psc_delta.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,8 @@ nest::pp_pop_psc_delta::calibrate()
temp = 0;
}

for ( int j = 0; j < V_.len_eta_; j++ )
// Set all except last state vector elements to zero, then fill last element with initial value
for ( int j = 0; j < V_.len_eta_ - 1; j++ )
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a very brutal fix. I looked some more at the pp_pop_psc_delta code and noticed (a) that the code is in poor shape and (b) that it has carried since Nov 2016 a deprecation notice referring users to gif_pop_psc_exp. I would therefore suggest to remove the model entirely. @jougs @terhorstd What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest to remove the model in 3.2 and add a corresponding sentence to the detailed release notes of 3.1.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to clarify: please add the deprecation info for the model to the release notes in this PR, and open a new issue with the 3.2 milestone set about the removal of the model.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @jougs. After some more code-staring, I now also understand why the -1 is correct above. Even though we will remove the model in 3.2, could you add a comment above like

# Set all except last element to zero.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Taking a look at the code in gif_pop_psc_exp, another way to fix out of bound access here would be:

    for ( int j = 0; j < V_.len_eta_; j++ )
    {
      S_.age_occupations_.push_back( 0 );
      S_.thetas_ages_.push_back( 0 );
      S_.n_spikes_ages_.push_back( 0 );
      S_.rhos_ages_.push_back( 0 );
    }
    S_.age_occupations_[ V_.len_eta_ - 1 ] = P_.N_;

I just don't know what would be the best comment for this code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Given that the code is soon disappearing, I'd keep changes minimal, so your -1 solution is fine. Just put this comment before the loop:

# Set all except last state vector elements to zero, then fill last element with initial value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it.

{
S_.age_occupations_.push_back( 0 );
S_.thetas_ages_.push_back( 0 );
Expand All @@ -319,7 +320,7 @@ nest::pp_pop_psc_delta::calibrate()
void
nest::pp_pop_psc_delta::update( Time const& origin, const long from, const long to )
{
assert( to >= 0 && ( delay ) from < kernel().connection_manager.get_min_delay() );
assert( to >= 0 and ( delay ) from < kernel().connection_manager.get_min_delay() );
assert( from < to );

for ( long lag = from; lag < to; ++lag )
Expand Down
12 changes: 6 additions & 6 deletions models/pulsepacket_generator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ nest::pulsepacket_generator::Parameters_::set( const DictionaryDatum& d, pulsepa
// We cannot use a single line here since short-circuiting may stop evaluation
// prematurely. Therefore, neednewpulse must be second arg on second line.
bool neednewpulse = updateValueParam< long >( d, names::activity, a_, node );
neednewpulse = updateValueParam< double >( d, names::sdev, sdev_, node ) || neednewpulse;
neednewpulse = updateValueParam< double >( d, names::sdev, sdev_, node ) or neednewpulse;
if ( a_ < 0 )
{
throw BadProperty( "The activity cannot be negative." );
Expand All @@ -89,7 +89,7 @@ nest::pulsepacket_generator::Parameters_::set( const DictionaryDatum& d, pulsepa
}


if ( updateValue< std::vector< double > >( d, "pulse_times", pulse_times_ ) || neednewpulse )
if ( updateValue< std::vector< double > >( d, "pulse_times", pulse_times_ ) or neednewpulse )
{
std::sort( pulse_times_.begin(), pulse_times_.end() );
ppg.B_.spiketimes_.clear();
Expand Down Expand Up @@ -152,7 +152,7 @@ nest::pulsepacket_generator::calibrate()
// determine pulse-center times that lie within
// a window sdev*sdev_tolerance around the current time
while (
V_.stop_center_idx_ < P_.pulse_times_.size() && P_.pulse_times_.at( V_.stop_center_idx_ ) - now <= V_.tolerance )
V_.stop_center_idx_ < P_.pulse_times_.size() and P_.pulse_times_.at( V_.stop_center_idx_ ) - now <= V_.tolerance )
{
if ( std::abs( P_.pulse_times_.at( V_.stop_center_idx_ ) - now ) > V_.tolerance )
{
Expand All @@ -179,7 +179,7 @@ nest::pulsepacket_generator::update( Time const& T, const long from, const long
if ( V_.stop_center_idx_ < P_.pulse_times_.size() )
{
while ( V_.stop_center_idx_ < P_.pulse_times_.size()
&& ( Time( Time::ms( P_.pulse_times_.at( V_.stop_center_idx_ ) ) ) - T ).get_ms() <= V_.tolerance )
and ( Time( Time::ms( P_.pulse_times_.at( V_.stop_center_idx_ ) ) ) - T ).get_ms() <= V_.tolerance )
{
V_.stop_center_idx_++;
}
Expand Down Expand Up @@ -214,13 +214,13 @@ nest::pulsepacket_generator::update( Time const& T, const long from, const long

// Since we have an ordered list of spiketimes,
// we can compute the histogram on the fly.
while ( not B_.spiketimes_.empty() && B_.spiketimes_.front() < ( T.get_steps() + to ) )
while ( not B_.spiketimes_.empty() and B_.spiketimes_.front() < ( T.get_steps() + to ) )
{
n_spikes++;
long prev_spike = B_.spiketimes_.front();
B_.spiketimes_.pop_front();

if ( n_spikes > 0 && prev_spike != B_.spiketimes_.front() )
if ( n_spikes > 0 and ( B_.spiketimes_.empty() or prev_spike != B_.spiketimes_.front() ) )
{
SpikeEvent se;
se.set_multiplicity( n_spikes );
Expand Down
4 changes: 3 additions & 1 deletion pynest/nest/lib/hl_api_helper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,9 @@
_deprecation_warning = {'deprecated_model': {'deprecation_issued': False,
'replacement': 'replacement_mod'},
'iaf_psc_alpha_canon': {'deprecation_issued': False,
'replacement': 'iaf_psc_alpha_ps'}}
'replacement': 'iaf_psc_alpha_ps'},
'pp_pop_psc_delta': {'deprecation_issued': False,
'replacement': 'gif_pop_psc_exp'}}


def format_Warning(message, category, filename, lineno, line=None):
Expand Down