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

Indexing out-of-bounds not always handled properly #776

Closed
syclik opened this issue Dec 24, 2020 · 10 comments · Fixed by #849
Closed

Indexing out-of-bounds not always handled properly #776

syclik opened this issue Dec 24, 2020 · 10 comments · Fixed by #849
Labels
bug Something isn't working

Comments

@syclik
Copy link
Member

syclik commented Dec 24, 2020

$ ./bin/stanc --version
stanc3 9df5448 (Unix)

When indexing a Stan array or vector with an index that's out-of-bounds, the Stan program terminates with different error conditions:

  • I would expect the program to catch the error, print a message, and terminate. This happens under some conditions.
  • Eigen assertion is triggered.
  • segmentation fault
  • no error -- looks like it's reading off the end of memory.

Examples

This triggers an Eigen assertion:

parameters {
  real theta;
}
transformed parameters {
  vector[0] x;
}
model {
  theta ~ normal(x[1], 1);
}

This seg faults (may not consistently seg fault):

transformed data {
  vector[0] x[2];
}
parameters {
  real theta;
}
model {
  theta ~ normal(x[0][1], 1);
}

This looks like it's reading off the end of memory:

transformed data {
  vector[0] x[2];
}
parameters {
  real theta;
}
model {
  theta ~ normal(x[3], 1);
}

I believe @bbbales2 has looked at the generated C++ and confirmed the issue.

@syclik syclik added the bug Something isn't working label Dec 24, 2020
@syclik
Copy link
Member Author

syclik commented Dec 24, 2020

edit: @SteveBronder has looked at the generated code.

@bbbales2
Copy link
Member

I think this is the same as the issue here: #489, that got fixed here: #521, which got reverted here cause it caused a performance regression: #656

Given that it causes segfaults, I kinda like the check back even if it causes noticeable slowdowns this time.

@seantalts
Copy link
Member

seantalts commented Dec 24, 2020

Ah, interesting! Is it the case that this only happens on index literals (eg y[0], not y[i]) like in the example here and in the first linked issue? Those we could check at compile time with no runtime performance cost. That would be neat.

@rok-cesnovar
Copy link
Member

Is it the case that this only happens on index literals (eg y[0], not y[i]) like in the example here and in the first linked issue?

Unfortunately not.

data {
    int N;
}
transformed data {
  vector[N] x[2];
}
parameters {
  real theta;
}
model {
  theta ~ normal(x[N][1], 1);
}

suffers from the same issue.

@seantalts
Copy link
Member

Only if N = 0?

@rok-cesnovar
Copy link
Member

Indeed.

@bbbales2
Copy link
Member

I ran into someone having this problem today. They were getting the error:

Chain 1 Assertion failed: (index >= 0 && index < size()), function operator[], file stan/lib/stan_math/lib/eigen_3.3.7/Eigen/src/Core/DenseCoeffsBase.h, line 408.

I'm into putting @nhuurre 's patch back in and then in the future someone can fix the performance thing with rvalue.

@bbbales2
Copy link
Member

bbbales2 commented Mar 9, 2021

@rybern I vaguely remember reading in an issue somewhere about rvalues and when to use them for assignments or not. Are you in r-value land now?

I forgot about this bug, but apparently it was fixed by forcing everything to go through rvalue (#521). We reverted that fix for performance reasons, but never got a fix for this in.

@rybern
Copy link
Collaborator

rybern commented Mar 10, 2021

@bbbales2 Not currently in r-value land, I'd have to spend some time catching up on this

@bbbales2
Copy link
Member

@rybern ah ah, I thought I remembered something about accessing arrays directly and that reminded me of this.

Anyway I guess it's something different so don't context switch into this just yet. Someone else might have a fix for this laying around. Will check in with @rok-cesnovar on it tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants