Fix crash from out-of-bounds copy in WaveClipSpectrumCache::GetSpectrogram#8974
Fix crash from out-of-bounds copy in WaveClipSpectrumCache::GetSpectrogram#8974haileys wants to merge 2 commits intoaudacity:audacity3from
Conversation
haileys
force-pushed
the
audacity3+fix-spectrogram-crash
branch
from
c849c75 to
bdc4ae2
Compare
haileys
force-pushed
the
audacity3+fix-spectrogram-crash
branch
from
bdc4ae2 to
21ee6d3
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a crash in WaveClipSpectrumCache::GetSpectrogram by tightening the range calculations for copying cached data, renaming Grow to Resize, and moving the resize call to avoid out-of-bounds access.
- Replaced
SpecCache::GrowwithResizeacross UI, header, and implementation. - Reworked
copyBegin/copyEndlogic to calculate a safe copy range. - Deferred the cache resize until after data is copied to preserve valid contents.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/tracks/playabletrack/wavetrack/ui/SpectrumView.cpp | Updated call from specCache.Grow to specCache.Resize to match renamed method. |
| src/tracks/playabletrack/wavetrack/ui/SpectrumCache.h | Renamed Grow to Resize in declaration and adjusted comment to reflect new behavior. |
| src/tracks/playabletrack/wavetrack/ui/SpectrumCache.cpp | Refactored out-of-bounds copy logic, moved Resize after memmove, and updated method name. |
Comments suppressed due to low confidence (1)
src/tracks/playabletrack/wavetrack/ui/SpectrumCache.cpp:535
- [nitpick] The comment indicates contents remain unchanged, but when shrinking the cache the data outside the new size range will be discarded. Clarify that only overlapping data is preserved.
// Resize the cache, keep the contents unchanged.
| )); | ||
| int copyLen = std::max(0, int(mSpecCache->len) - std::abs(oldX0)); | ||
| copyBegin = oldX0 >= 0 ? 0 : std::abs(oldX0); | ||
| copyEnd = copyBegin + copyLen; |
There was a problem hiding this comment.
copyBegin and copyEnd are not clamped to numPixels, which could lead to copying beyond buffer if the number of destination pixels is smaller than the cache length. Consider clamping copyBegin and copyEnd to [0, numPixels].
| copyEnd = copyBegin + copyLen; | |
| copyEnd = copyBegin + copyLen; | |
| copyBegin = std::clamp(copyBegin, 0, numPixels); | |
| copyEnd = std::clamp(copyEnd, 0, numPixels); |
There was a problem hiding this comment.
This AI slop is wrong, copying beyond the end of the buffer is exactly what this PR fixes.
| void Grow( | ||
| // Resize the cache, while preserving the (possibly now invalid!) contents if growing | ||
| void Resize( | ||
| size_t len_, SpectrogramSettings& settings, double samplesPerPixel, |
There was a problem hiding this comment.
[nitpick] Parameter 'settings' is passed by non-const reference, but the method does not modify it. Consider changing it to 'const SpectrogramSettings&' for const-correctness.
| size_t len_, SpectrogramSettings& settings, double samplesPerPixel, | |
| size_t len_, const SpectrogramSettings& settings, double samplesPerPixel, |
There was a problem hiding this comment.
This AI slop is also wrong. This method does modify settings. Unbelievable
|
I'll send this one to Tenacity instead. |
|
@haileys, I approved your PR, and it was assigned to QA for testing before it could be merged. No decisions were made based on AI "analysis". In any case we respect your choice, and thanks for the report. |
|
@kryksyh It's quite disrespectful to send volunteer contributors onto a wild goose hunt of AI slop "reviews". Whether or not decisionmaking is based on it in the end, what you are communicating to contributors is "you need to fix these things that the AI told you about", which is not at all respectful of their time. (I'll elide all the other technical and moral issues with "AI" since people have written extensively about those already) |
Yes, blame on me, you are right to point this out and I had to "resolve" those messages when I checked them and saw that they are incorrect. On the other hand, the PR was approved which is a direct communication that no further action is required. |
|
is there a way then to turn off copilot's "helpful" diagnostics when the PR is already approved then? |
Resolves: #8851
This fixes a crash bug in
WaveClipSpectrumCache::GetSpectrogram. I've cleaned up the arithmetic for calculating the begin and end indices for copying cached spectrogram data. PreviouslycopyBegin + oldX0could exceedmSpecCache->lenafter shrinkingmSpecCache, and this is where the crash on out of bounds access was coming from.I've also renamed
SpecCache::GrowtoSpecCache::Resizeto more accurately reflect what this function does, while also moving its invocation to after the crashing memmove to guarantee that we are never indexing out of bounds.Recommended: