search

david-cortes / outliertree

(Python, R, C++) Explainable outlier/anomaly detection through decision tree conditioning
http://outliertree.readthedocs.io
GNU General Public License v3.0
56 stars 4 forks source link

Possible UB that can lead to a crash due to assertions in GLIBC #3

Closed danilaml closed 3 years ago

danilaml commented 3 years ago

The code uses lot's of constructs like &vec[0] such as here: https://github.com/david-cortes/outliertree/blob/master/src/fit_model.cpp#L192

This is problematic and may cause assertions on some configurations that are built with certain assertions enabled. See similar issue here: https://github.com/rstudio/httpuv/issues/133

david-cortes commented 3 years ago

Thanks for the bug report. Seeing that would be very problematic, since the code is meant to make a check to ensure that the vector has at least one element beforehand whenever it does that, and changing it to std::vector::data() will instead lead to a different error.

Did you get that assertion as a run-time warning when testing the package, or was it just a static code analysis hint?

danilaml commented 3 years ago

@david-cortes I was motivated by this SO question: https://stackoverflow.com/questions/66892158/assertion-builtin-expect-n-this-size-true-failed-error-in-visual

I'm not sure what the supposed issue with std::vector::data() would be. If the code actually used the &v[0] pointer anywhere without checking whether v,size() > 0 it was already wrong (and otherwise the result should be identical).

Note, that the line I've linked is not necessarily what causes this error since there was no trace available. It's just an example of a potentially problematic one, since I've noticed it's used a lot in that file and some such usage appears to be triggering the assert.

Perhaps you'll be able to reproduce it using the code from SO.

david-cortes commented 3 years ago

Thanks, got the context now. I'm able to reproduce it after enabling the macro _GLIBCXX_ASSERTIONS. However the error is not with the &vector[0] parts as changing them to &vec.at(0) doesn't throw any error. Will investigate where it's failing.

david-cortes commented 3 years ago

Now I'm pretty sure the issue is solved.