-
-
Notifications
You must be signed in to change notification settings - Fork 258
Conversation
Removed two looping usages of `alloca` that could lead to smashed stacks. Signed-off-by: Emily Mabrey <emabrey@tenacityaudio.org>
alloca
usage (CWE-770)alloca
usage (CWE-770)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Learn RAII please.
WaveTrack **chans = (WaveTrack **) alloca(numPlaybackChannels * sizeof(WaveTrack *)); | ||
float **tempBufs = (float **) alloca(numPlaybackChannels * sizeof(float *)); | ||
auto chans = new WaveTrack * [numPlaybackChannels]; | ||
auto tempBufs = new float* [numPlaybackChannels]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Friendly words of advice: no naked new! And (almost) never write delete (unless inside a destructor of your own smart pointer, or some cases where delete this
is needed).
I worked hard to banish them from Audacity. Don't let them creep back into your fork. Use proper RAII.
auto chans = std::make_unique<WaveTrack*[]>(numPlaybackChannels);
auto tempBufs = std::make_unique<std::unique_ptr<float[]>[]>(numPlaybackChannels);
for (unsigned int c = 0; c < numPlaybackChannels; c++) | ||
tempBufs[c] = (float *) alloca(framesPerBuffer * sizeof(float)); | ||
for (unsigned int c = 0; c < numPlaybackChannels; c++) { | ||
tempBufs[c] = new float[framesPerBuffer]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Memory leak. Tsk tsk.
To compile with my other suggested change:
tempBufs[c] = std::make_unique<float[]>(framesPerBuffer);
And then the missing delete
for this new
would not matter.
But then other things are needed to fix the compilation too. Exercise for you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See line 4006: delete[] tempBufs;
I don't believe it's missing. Additionally, the reason they are all the way down there at the bottom is to match the way that alloca
deallocates from the stack at the end of the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was incorrect, I missed a level on the essentially 3D array.
This was a good catch. I'm used to Java style arrays where On a side note, I appreciate the constructive criticism. |
Rework `alloca` fix to use new helper class for buffer allocations. Add missing `delete[]` for cleanup of the temporary playback buffer. Signed-off-by: Emily Mabrey <emabrey@tenacityaudio.org> Reference-to: https://github.com/tenacityteam/tenacity/issues/416 Reference-to: https://github.com/tenacityteam/tenacity/pull/412 Helped-by: Paul Licameli
* Rework `alloca` fix to use new helper class for buffer allocations. * Add missing `delete[]` for cleanup of the temporary playback buffer. Signed-off-by: Emily Mabrey <emabrey@tenacityaudio.org> Reference-to: https://github.com/tenacityteam/tenacity/issues/416 Reference-to: https://github.com/tenacityteam/tenacity/pull/412 Helped-by: Paul Licameli
Constructive criticism is the highest compliment. It means work is taken seriously. We must all learn to give it and take it with grace. |
The first rule of audio programming is no time unbounded behavior in the audio thread. That means no heap allocation. Not sometimes, zero -- never. The worst case scenario will happen sometimes and the result is audible glitching. I was unfamiliar with |
Stack "allocation" happens all the time and isn't so great a concern. alloca is a nonstandard function implemented as a compiler intrinsic on all of the few compilers that matter, for a stack allocation of a size not known statically, and it just changes a few registers. (And may specially affect compilation of a function containing it.) C99 diverged from C++ and made variable-length arrays a feature, making alloca unnecessary -- but then C11 made that language feature optional. https://en.wikipedia.org/wiki/C11_(C_standard_revision)#Optional_features |
Removed two looping usages of
alloca
that could lead to smashed stacks. This may come at a very small cost to latency in some cases, but it helps with program stability, as any errors or issues withalloca
usage causes UB/stack overflows. When used in a loop it is incredibly easy to smash the stack, so it was identified as ahigh
security issue. Additionally, usage ofalloca
thwarts inlining, as it is never safe for the compiler to inlinealloca
containing functions. So, in some cases, rather than an increase in latency, the compiler may now be able to provide a decrease due to increased ability to inline functions.Signed-off-by: Emily Mabrey emabrey@tenacityaudio.org
Checklist
-s
orSigned-off-by
* (See: Contributing § DCO)* indicates required