Unsafe use of getWritePointer()


#1

In the develop branch, juce_AudioFormatReader.cpp:171

if (float* const d = buffer->getWritePointer (j, startSample))
FloatVectorOperations::convertFixedToFloat (d, reinterpret_cast<const int*> (d), 1.0f / 0x7fffffff, numSamples);’

getWritePointer() doesn’t validate it’s parameters, so the return value is garbage if called with invalid parameters.

So testing it with a if statement is pointless.


#2

You are right, that technically the if does not make any sense - even though the parameters are checked with asserts. But I would still argue that it is cleaner code to always check pointer return values with such an if statement unless the function is super performance critical.

The reason is that the API does not explicitly say that it doesn’t check the parameters internally, so it may do this in the future. So the code in AudioFormatReader is more future proof.

After much of Jules’ preaching :slight_smile:, I now tend to always automatically write the if statement with any function that returns a pointer.


#3

But if you call AudioFormatReader::read (&buffer, buffer.getNumSamples() + 1, 1, 0, true, true) it will still crash although the if suggests that passing an out of bounds start offset is savely caught.


#4

I think it’s there to deal with the case where you pass an array of pointers, some of which are null to indicate that you don’t want to read that particular track. You might do that if for example you have a multi-track audio file, but only want to extract a couple of tracks from it and avoid copying data from all the others.

However, I’m not sure whether it’s technically legal to create an AudioBuffer from an array of pointers if some of them are null… that could break other stuff, and should maybe be something we assert about?

(Also not sure why the AudioSampleBuffer parameter is a pointer rather than a reference - we should update that. It’s clearly a method that’s been around for some time!)