swesterfeld / spectmorph

SpectMorph: spectral audio morphing
http://www.spectmorph.org
GNU Lesser General Public License v2.1
64 stars 5 forks source link

LIB: fix invalid access to past last element #24

Closed hfiguiere closed 9 months ago

hfiguiere commented 9 months ago

The plugin was aborting on load when built as a flatpak. (default CXX flags have C++ assertions enabled)

swesterfeld commented 9 months ago

Yes, I agree this is UB and needs to be fixed. From a quick look at the source code, this is not the only place where &foo[foo.size()] is used. I'll try to find the other spots as well and fix it everywhere. Also I think that &foo[0] is also a problem because it will cause out-of-bounds errors if the vector is empty, so this probably also needs fixing by using the data() function of std::vector instead, which can also be used for empty vectors.

Interestingly the address sanitizer and UB sanitizer don't really find this bug, but I can reproduce your "crash" by compiling with -D_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC.

hfiguiere commented 9 months ago

Also I think that &foo[0] is also a problem because it will cause out-of-bounds errors if the vector is empty

That one too.

Maybe MMapIn::open_mem could take iterators that return the proper type.

swesterfeld commented 9 months ago

I ended up with adding a method MMapIn::open_vector which does the right thing. The code is merged now (https://github.com/swesterfeld/spectmorph/commit/d9454670670c17ec7a9b3e348e07fb3a08c2c443), I hope I found every spot in the code that needed fixing. At least a bit of testing worked for me.

hfiguiere commented 9 months ago

BTW -Wp,-D_GLIBCXX_ASSERTIONS in CXXFLAGS is enough to trigger it. And UB sanitizer doesn't know about the C++ library. This is the semantincs of the [] operator.

Thanks !