Jassert in AudioBuffer::copyFrom for negative numSamples


#1

Hi everyone,
Somehow I managed to copy a negative amount of samples via copyFrom and ended up with an ugly memory corruption, that was hard to track down.

Can we get another asserts for numSamples being positive?

…and the similar occasions…


#2

Calling any of the AudioBuffer functions with a negative number of samples is not undefined behaviour - it just won’t have any effect.


#4

Disregard my previous post, I misinterpreted the code posted by the OP. Need coffeee…


#5

Not quite. The argument is an int, which does a raw cast to a size_t. So we are copying quite a bit of memory and certainly not in the limit.

And the jasserts in copyFrom asserting that the int is intact. In my case I ended up with this unlucky case:

buffer.copyFrom (0, 32, buffer2, 0, 32, -32);

jassert (destStartSample >= 0 && destStartSample + numSamples <= size);  // doesn't blow, 0 <= 32 + -32

But it is copying 18446744073709551584 * sizeof(float) bytes. That’s not really what I call no effect :wink:


#6

Yep, those functions could use some assertions!


#7

But FloatVectorOperations::copy is always guarded with

if (numSamples > 0)

somewhere earlier in the call stack - you can see an example in your first post. How are you getting through?


#8

Fair point, maybe it wasn’t the copyFrom. I didn’t actually check (because the memory corruption was screwing up in a different call stack). I rather fixed my math not to send negative numbers.

But I researched it now, /probably/ it was applyGain here:

ending up here, where vDSP_Length is an “unsigned long”:

Edited: not line 722 but line 758 in FloatVectorOperations.cpp - different arguments…


#9

I’ve already looked at this and added a bunch of assertions. There were a few methods where negative numbers were safe, others where they weren’t - I’ve just added assertions everywhere because I think you’d always want to find out about it if your code starts passing around numbers like that.

This would be such a great place for us to use a paradigm like array_view… We really must add something like that to the library and it’ll simplify a whole heap of this kind of code.