Pitch stretching with mid/side processing can cause a heap buffer overflow under certain conditions

[psobot]

Hello again! I think I've run across a very rare edge case with Rubber Band. I seem to be able to consistently make Rubber Band write past the end of an internal buffer and cause a crash when the following conditions are true:

• Pitch stretching is being used
• Audio is passed in fixed-size chunks to process (i.e.: passing in more than getSamplesRequired())
• OptionProcessRealTime, OptionChannelsTogether and PitchHighSpeed are all passed
• Two channels of audio are passed
• The size of each audio buffer exceeds 4,096 samples

In this case, Address Sanitizer reports a heap buffer overflow past the end of the ms buffer:

==30666==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x629000040200 at pc 0x00010b95f33e bp 0x7ffee42f55e0 sp 0x7ffee42f55d8
WRITE of size 4 at 0x629000040200 thread T0
    #0 0x10b95f33d in RubberBand::RubberBandStretcher::Impl::prepareChannelMS(unsigned long, float const* const*, unsigned long, unsigned long, float*) StretcherProcess.cpp:160
    #1 0x10b95c649 in RubberBand::RubberBandStretcher::Impl::consumeChannel(unsigned long, float const* const*, unsigned long, unsigned long, bool) StretcherProcess.cpp:216
    #2 0x10b9309e1 in RubberBand::RubberBandStretcher::Impl::process(float const* const*, unsigned long, bool) StretcherImpl.cpp:1335
    #3 0x10b92fed6 in RubberBand::RubberBandStretcher::process(float const* const*, unsigned long, bool) RubberBandStretcher.cpp:146

0x629000040200 is located 0 bytes to the right of 16384-byte region [0x62900003c200,0x629000040200)
allocated by thread T0 here:
    #0 0x10249c17d in wrap_malloc+0x9d (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x4917d)
    #1 0x102275f43 in float* RubberBand::allocate<float>(unsigned long) Allocators.h:84
    #2 0x1022a3604 in float* RubberBand::allocate_and_zero<float>(unsigned long) Allocators.h:165
    #3 0x1022a28d7 in RubberBand::RubberBandStretcher::Impl::ChannelData::construct(std::__1::set<unsigned long, std::__1::less<unsigned long>, std::__1::allocator<unsigned long> > const&, unsigned long, unsigned long, unsigned long) StretcherChannelData.cpp:91
    #4 0x1022a30f8 in RubberBand::RubberBandStretcher::Impl::ChannelData::ChannelData(std::__1::set<unsigned long, std::__1::less<unsigned long>, std::__1::allocator<unsigned long> > const&, unsigned long, unsigned long, unsigned long) StretcherChannelData.cpp:48
    #5 0x1022a3174 in RubberBand::RubberBandStretcher::Impl::ChannelData::ChannelData(std::__1::set<unsigned long, std::__1::less<unsigned long>, std::__1::allocator<unsigned long> > const&, unsigned long, unsigned long, unsigned long) StretcherChannelData.cpp:47
    #6 0x1022aabfe in RubberBand::RubberBandStretcher::Impl::configure() StretcherImpl.cpp:656
    #7 0x1022a8b59 in RubberBand::RubberBandStretcher::Impl::Impl(unsigned long, unsigned long, int, double, double) StretcherImpl.cpp:181
    #8 0x1022858ee in RubberBand::RubberBandStretcher::Impl::Impl(unsigned long, unsigned long, int, double, double) StretcherImpl.cpp:115
    #9 0x10228584b in RubberBand::RubberBandStretcher::RubberBandStretcher(unsigned long, unsigned long, int, double, double) RubberBandStretcher.cpp:35
    #10 0x10228593e in RubberBand::RubberBandStretcher::RubberBandStretcher(unsigned long, unsigned long, int, double, double) RubberBandStretcher.cpp:37
    #11 0x1022c6fac in main rubberband_block_size_test.cpp:12
    #12 0x7fff20418f3c in start+0x0 (libdyld.dylib:x86_64+0x15f3c)

The following test harness seems to reliably reproduce the above crash (only tested on my machine so far, macOS x86):

#include "./vendors/rubberband/single/RubberBandSingle.cpp"

using namespace RubberBand;

// Setting blockSize here to anything bigger than 4096 causes a crash!
static const int blockSize = 8192;
static const float sampleRate = 44100;
static const int numChannels = 2;
static const int numIterations = 2;

int main() {
  RubberBandStretcher stretcher(sampleRate, numChannels,
    RubberBandStretcher::OptionProcessRealTime |
    RubberBandStretcher::OptionThreadingNever |
    RubberBandStretcher::OptionChannelsTogether |
    RubberBandStretcher::OptionPitchHighSpeed);
  stretcher.setPitchScale(1.1);

  // The presence of this line seems to have no effect:
  stretcher.setMaxProcessSize(blockSize);

  float *inChannels[numChannels] = {
    (float *)malloc(sizeof(float) * blockSize),
    (float *)malloc(sizeof(float) * blockSize),
  };

  // This should cause an overflow (and crash if ASan is enabled)
  stretcher.process(inChannels, blockSize, false);

  printf("If you got here, no overflow! Congratulations!\n");
}

Please let me know if you need any more details. (I think it should be possible to work around this crash by only passing at most getSamplesRequired() samples per call to process, but I haven't experimented too much further with that yet.)

[cannam]

Ouch! Thank you, I will look into it.

[cannam]

I've pushed a fix, and added to my local test set a program to exercise this situation with an exhaustive set of option combinations. Please test!

The way memory is managed throughout is decidedly antique and it's on my slate to revise it, but an immediate fix is far more important. I'll make another release with this fix in the coming days.

Thanks again for the very clear report and test case.

[psobot]

Thanks @cannam! That was incredibly fast - I can confirm that this no longer crashes, either in the provided test case or in my application. Much appreciated!