Heap buffer overflow in bfastts

[GreatEmerald]

See CRAN check results on ASAN:

https://www.stats.ox.ac.uk/pub/bdr/memtests/gcc-ASAN/bfast/00check.log

We probably need some additional sanity checks here?

[GreatEmerald]

At least one issue seems to be that it's calling b[j] where j=1, but it never checks whether b even has an element 1, which is not guaranteed here. I'm not sure if that is enough to fix the issue, though.

[GreatEmerald]

Hmm, actually, wouldn't just switching the two around fix the above issue? I.e.:

while(a[i] >= b[j] && j < nb) ++j; 

To:

while(j < nb && a[i] >= b[j]) ++j; 

On the other hand, if nb < 1, then this would still have j = 1 and thus it would probably fail down the line at if (fabs(a[i] - b[j]) < fabs(a[i]-b[j-1])) {, so we probably still need to have a specific check for this or so.

[GreatEmerald]

I'm realising that probably the issue is more so at the end than at the start, and for the end part indeed the above should fix it. I think if e.g. nb = 8, the iteration j = 7 would lead to an increase of j to 8, in which case the next iteration checks b[j] when j == 8 (and that's too much due to C++ being 0-based).

[appelmar]

Thanks @GreatEmerald! My guess is that switching the expressions can solve this, I'll try out with the rocker/r-devel-san image!

[GreatEmerald]

It seems that you can also check with valgrind, it detects the same issue.

[GreatEmerald]

Also, it may be in any case a good idea to add some sanity checks, e.g. if we have empty vectors etc.

[appelmar]

Sure, I use ASAN /UBSAN etc. instead of valgrind just by habit because of speed... I can also add some checks to the function.

[appelmar]

Quick update: I haven't been successful to reproduce this so far. With rocker/r-devel-san, it fails to install the package dependencies (same for R hub builder) and valgrind did not report any issue on my machine, but I have to try with different compiler flags.

[GreatEmerald]

Thanks for the update. The flags that CRAN is using are described here, it should be possible to reproduce their setup: https://www.stats.ox.ac.uk/pub/bdr/memtests/README.txt

[appelmar]

Thanks, I didn't know about the --use-valgrind flag for R CMD check...

[appelmar]

I think https://github.com/bfast2/bfast/commit/63912315bcdccb900b6e26598bdd76739314334b should fix the issue (see PR). I've also compared all examples to the fallback implementation of bfastts.

[GreatEmerald]

Great, thanks! I left one comment there.