Skip to content
This repository has been archived by the owner on Dec 18, 2022. It is now read-only.

Fix for unsafe alloca usage (CWE-770) #412

Merged
merged 1 commit into from
Jul 30, 2021
Merged

Fix for unsafe alloca usage (CWE-770) #412

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions src/AudioIO.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3844,12 +3844,13 @@ bool AudioIoCallback::FillOutputBuffers(

// ------ MEMORY ALLOCATION ----------------------
// These are small structures.
WaveTrack **chans = (WaveTrack **) alloca(numPlaybackChannels * sizeof(WaveTrack *));
float **tempBufs = (float **) alloca(numPlaybackChannels * sizeof(float *));
auto chans = new WaveTrack * [numPlaybackChannels];
auto tempBufs = new float* [numPlaybackChannels];
Copy link
Contributor

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);


// And these are larger structures....
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];
Copy link
Contributor

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.

Copy link
Member Author

@emabrey emabrey Jul 31, 2021 •
edited
Loading

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.

Copy link
Member Author

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.

}
// ------ End of MEMORY ALLOCATION ---------------

auto & em = RealtimeEffectManager::Get();
Expand Down Expand Up @@ -4001,6 +4002,8 @@ bool AudioIoCallback::FillOutputBuffers(
if (outputMeterFloats != outputFloats)
ClampBuffer( outputMeterFloats, framesPerBuffer*numPlaybackChannels );

delete[] chans;
delete[] tempBufs;
return false;
}

Expand Down
19 changes: 13 additions & 6 deletions src/effects/RealtimeEffectManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,15 +323,15 @@ size_t RealtimeEffectManager::RealtimeProcess(int group, unsigned chans, float *
wxMilliClock_t start = wxGetUTCTimeMillis();

// Allocate the in/out buffer arrays
float **ibuf = (float **) alloca(chans * sizeof(float *));
float **obuf = (float **) alloca(chans * sizeof(float *));
auto ibuf = new float* [chans];
auto obuf = new float* [chans];

// And populate the input with the buffers we've been given while allocating
// NEW output buffers
for (unsigned int i = 0; i < chans; i++)
{
ibuf[i] = buffers[i];
obuf[i] = (float *) alloca(numSamples * sizeof(float));
obuf[i] = new float[numSamples];
}

// Now call each effect in the chain while swapping buffer pointers to feed the
Expand Down Expand Up @@ -366,6 +366,9 @@ size_t RealtimeEffectManager::RealtimeProcess(int group, unsigned chans, float *
}
}

delete ibuf;
delete[] obuf;

// Remember the latency
mRealtimeLatency = (int) (wxGetUTCTimeMillis() - start).GetValue();

Expand Down Expand Up @@ -516,9 +519,10 @@ size_t RealtimeEffectState::RealtimeProcess(int group,
const auto numAudioIn = mEffect.GetAudioInCount();
const auto numAudioOut = mEffect.GetAudioOutCount();

float **clientIn = (float **) alloca(numAudioIn * sizeof(float *));
float **clientOut = (float **) alloca(numAudioOut * sizeof(float *));
float *dummybuf = (float *) alloca(numSamples * sizeof(float));
auto clientIn = new float* [numAudioIn];
auto clientOut = new float* [numAudioOut];
auto dummybuf = new float [numSamples];

decltype(numSamples) len = 0;
auto ichans = chans;
auto ochans = chans;
Expand Down Expand Up @@ -613,6 +617,9 @@ size_t RealtimeEffectState::RealtimeProcess(int group,
// Bump to next processor
processor++;
}
delete[] clientIn;
delete[] clientOut;
delete[] dummybuf;

return len;
}
Expand Down