Importer fixes - #34448
Open
miiizen wants to merge 12 commits into
Open
Conversation
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
📝 WalkthroughWalkthroughThe changes add input validation across BB, Capella, and Guitar Pro importers. BB parsing checks buffer ranges, event boundaries, and chord indices. Capella validates lengths, key offsets, staff layouts, file reads, and conversion errors. Guitar Pro validates MIDI channel and dynamic indices before array access. Fixed-size 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/importexport/bb/internal/bb.cpp`:
- Around line 601-607: Move the c.root range validation ahead of the Harmony* h
and HarmonyInfo* info allocations in the chord-processing loop. For invalid
roots, keep logging the error, setting TPC_INVALID, and continuing without
allocating either object; preserve the existing initialization and root
assignment for valid chords.
- Around line 423-426: Update the BB event decoding around the 12-byte range
check to build both the event length and tick fields using unsigned or wider
intermediates, preventing signed shifts when high bytes are present. Validate
the decoded length against the destination int range before assigning it, and
perform duration scaling in a widened type before storing the result.
In `@src/importexport/capella/internal/capella.cpp`:
- Around line 2868-2869: Preserve conversion failures in both import paths: in
src/importexport/capella/internal/capella.cpp lines 2868-2869, update the binary
conversion handling around convertCapella to catch conversion errors, clean up
partial score state, and return a failure status; apply the same behavior in
src/importexport/capella/internal/capxml.cpp lines 1438-1444 for XML conversion.
Ensure malformed input never returns Err::NoError after an exception.
- Around line 1489-1496: Apply one shared allocation-safety limit to the length
values returned by readUnsigned() before every payload allocation: validate size
before the std::vector<char> allocation in
src/importexport/capella/internal/capella.cpp:1489-1496 and validate len before
the raw new char[] allocation at
src/importexport/capella/internal/capella.cpp:2164-2171. Preserve BAD_FORMAT
handling while ensuring malformed or truncated length prefixes cannot result in
oversized allocations.
In `@src/importexport/guitarpro/internal/importgtp-gp4.cpp`:
- Around line 726-729: The failed-read paths leak gp before returning false. In
src/importexport/guitarpro/internal/importgtp-gp4.cpp:726-729, release gp when
GuitarPro4::read validation fails; in
src/importexport/guitarpro/internal/importgtp-gp5.cpp:569-572, release gp when
GuitarPro5::readTracks validation fails, preserving the existing false returns.
In `@src/importexport/guitarpro/internal/importgtp.h`:
- Line 324: Update the legacy GuitarPro2::read and GuitarPro3::read paths to
validate midiChannel with a >= 0 and < channelDefaults.size() guard before
indexing channelDefaults or reading patch, volume, pan, chorus, and reverb
values. Match the existing validation behavior used by the GP4/GP5 readers,
preserving normal processing for valid channels.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 05df91a9-155d-47e6-aeef-a9d588d1df60
📒 Files selected for processing (9)
src/importexport/bb/internal/bb.cppsrc/importexport/bb/internal/bb.hsrc/importexport/capella/internal/capella.cppsrc/importexport/capella/internal/capella.hsrc/importexport/capella/internal/capxml.cppsrc/importexport/guitarpro/internal/importgtp-gp4.cppsrc/importexport/guitarpro/internal/importgtp-gp5.cppsrc/importexport/guitarpro/internal/importgtp.cppsrc/importexport/guitarpro/internal/importgtp.h
Comment on lines
+423
to
+426
| if (idx + 12 > EVENT_END_BOUNDARY) { | ||
| LOGE() << "Event out of range"; | ||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=src/importexport/bb/internal/bb.cpp
if rg -n 'a\[idx \+ [0-9]+\]\s*<<\s*24' "$file"; then
echo "Uncast 32-bit event shifts remain" >&2
exit 1
fiRepository: musescore/MuseScore
Length of output: 425
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=src/importexport/bb/internal/bb.cpp
echo "== outline around target =="
sed -n '400,470p' "$file" | cat -n
echo
echo "== includes / constants =="
rg -n 'Constants::DIVISION|class Constants|enum.*DIVISION|`#include`' "$file" src/importexport/bb/internal src -g '*bb*.cpp' -g '*bb*.h' | head -80
echo
echo "== Fraction fromTicks occurrences =="
rg -n 'fromTicks|lengthTicks|Fraction\b' src/framework src/utils src/importexport/bb -g '*.cpp' -g '*.h' | head -120Repository: musescore/MuseScore
Length of output: 15502
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=src/importexport/bb/internal/bb.cpp
echo "== class/declaration context =="
sed -n '1,60p' "$file" | cat -n
echo
echo "== Constant definitions =="
rg -n 'DIVISION|class Constants|struct Constants|BBFile::read.*{|\bread\(' src/importexport/bb src/engraving/dom -g '*.cpp' -g '*.h' | head -120
echo
echo "== Fraction tick/length behavior probe =="
python3 - <<'PY'
def c_shift(byte, width):
# C/C++ promotes a uchar argument with << to int; left shift of negative int is UB.
v = int.from_bytes(bytes([byte]), signed=False) if width in {8,16,24} else byte
if width == 24:
y = 1 if v >= 128 else 0
return ((-2**31) >> (31 - width)) | (v << width) if y else (v << width)
return v << width
for b in (0, 1, 127, 128, 255):
raw = b + (128 << 8) + (64 << 16) + (128 << 24)
signed_raw = (b + (128 << 8) + (64 << 16) + (128 << 24)) % 2**32
if signed_raw >= 2**31:
signed_raw -= 2**32
print({"byte": b, "math_combination": raw, "int32_combination": signed_raw})
PYRepository: musescore/MuseScore
Length of output: 2724
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=src/importexport/bb/internal/bb.cpp
echo "== bb.h relevant lines =="
sed -n '1,90p' src/importexport/bb/internal/bb.h | cat -n
echo
echo "== DIVISION occurrences =="
rg -n 'DIVISION|class Constants|struct Constants|BBFile::read|bool read' src/importexport/bb src -g '*.cpp' -g '*.h' | head -200
echo
echo "== Fraction class definitions/usages relevant =="
rg -n 'class Fraction|Fraction::fromTicks|setOntime|setDuration' src/engraving src -g '*.cpp' -g '*.h' | head -160Repository: musescore/MuseScore
Length of output: 41330
Use unsigned or wider arithmetic for decoded BB event values.
The 12-byte range check is correct, but a[idx + 8] is still promoted to signed int before << 24 at line 453. A high event length byte can make that shift undefined behavior. Decode the length through an unsigned value, reject values that exceed the destination type before assigning to int, and perform duration scaling in a widened type. Decode the tick field the same way at line 442 to avoid << 24 from a high event byte.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/importexport/bb/internal/bb.cpp` around lines 423 - 426, Update the BB
event decoding around the 12-byte range check to build both the event length and
tick fields using unsigned or wider intermediates, preventing signed shifts when
high bytes are present. Validate the decoded length against the destination int
range before assigning it, and perform duration scaling in a widened type before
storing the result.
Comment on lines
+601
to
+607
| if (c.root <= 0 || c.root > table.size()) { | ||
| LOGE() << "Chord root out of range"; | ||
| info->setRootTpc(Tpc::TPC_INVALID); | ||
| continue; | ||
| } | ||
| info->setRootTpc(table[c.root - 1]); | ||
| if (c.bass > 0) { | ||
| if (c.bass > 0 && c.bass <= table.size()) { |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 'createHarmony|new HarmonyInfo|root out of range|s->add\(h\)' srcRepository: musescore/MuseScore
Length of output: 46096
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Outline harmony info files:"
fd -i 'harmony.*\.(h|cpp)$' src | sed 's#^\./##' | while read -r f; do rg -n 'class HarmonyInfo|HarmonyInfo::~HarmonyInfo|void setHarmony|std::vector<HarmonyInfo>' "$f" || true; done
echo
echo "bb.cpp slice:"
sed -n '570,620p' src/importexport/bb/internal/bb.cpp | cat -n
echo
echo "HarmonyInfo definition/destructor:"
rg -n -C 8 'class HarmonyInfo|~HarmonyInfo|void HarmonyInfo::setHarmony|void HarmonyInfo' src/engraving src/common -g '*harmony*.h' -g '*harmony*.cpp' -g '*types*.h' | head -n 240Repository: musescore/MuseScore
Length of output: 5369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Ownership-related chord list declarations:"
rg -n -C 4 'std::vector<.*HarmonyInfo|ChordList|addChord|setRootTpc|setBassTpc' src/engraving src/common src/importexport | head -n 240Repository: musescore/MuseScore
Length of output: 15973
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Factory createHarmony declarations/definitions:"
rg -n -C 12 'Harmony\* Factory::createHarmony|createHarmony\(Segment|delete h|~Harmony' src/engraving/dom/factory.cpp src/engraving/dom/harmony.cpp src/engraving/dom/element.h src/engraving/dom/element.cpp | head -n 260
echo
echo "Parent ownership references around Harmony add/delete:"
rg -n -C 5 'class Segment|~Segment|add\(EngravingItem|removeElement|delete.*children|deleteChildren|HarmonyInfo::addChord|m_chords' src/engraving/dom src/engraving | head -n 320Repository: musescore/MuseScore
Length of output: 3020
Validate the chord root before allocating harmony objects.
When c.root is out of range, Harmony* h and HarmonyInfo* info are allocated before the validation, then the loop continues without adding them to the segment. Move the root check before creating these objects so malformed chords do not leave allocated harmony objects unattached.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/importexport/bb/internal/bb.cpp` around lines 601 - 607, Move the c.root
range validation ahead of the Harmony* h and HarmonyInfo* info allocations in
the chord-processing loop. For invalid roots, keep logging the error, setting
TPC_INVALID, and continuing without allocating either object; preserve the
existing initialization and root assignment for valid chords.
Comment on lines
+1489
to
+1496
| if (size == std::numeric_limits<unsigned>::max()) { | ||
| throw Capella::Error::BAD_FORMAT; | ||
| } | ||
| std::vector<char> vtxt(size + 1); | ||
| char* txt = vtxt.data(); | ||
| cap->read(txt, size); | ||
| if (!cap->read(txt, size)) { | ||
| throw Capella::Error::BAD_FORMAT; | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate capella files:"
fd -a 'capella\.(cpp|h|hpp|ino)$' . || true
echo
echo "Git diff stat:"
git diff --stat || true
echo
echo "Search readUnsigned/read len/usages:"
rg -n "readUnsigned|readUnsigned|BadFormat|BAD_FORMAT|vtxt|new char\\[static_cast<.*len|numeric_limits<unsigned>::max" -S .
echo
echo "Show target ranges with context:"
for f in src/importexport/capella/internal/capella.cpp; do
if [ -f "$f" ]; then
echo "--- $f lines 1460-1515 ---"
sed -n '1460,1515p' "$f" | nl -ba -v1460
echo "--- $f lines 2140-2185 ---"
sed -n '2140,2185p' "$f" | nl -ba -v2140
fi
done
echo
echo "Find read(unsigned) implementations:"
rg -n "bool .*read\\(.*char\\*|unsigned .*readUnsigned|size_t .*readUnsigned|readUnsigned|read\\(.*unsigned" src/importexport/capella/internal -SRepository: musescore/MuseScore
Length of output: 8475
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- capella.cpp lines 1480-1498 ---"
sed -n '1480,1498p' src/importexport/capella/internal/capella.cpp
echo "--- capella.cpp lines 2110-2172 ---"
sed -n '2110,2172p' src/importexport/capella/internal/capella.cpp
echo "--- capella related read declarations and capellareader files ---"
sed -n '730,785p' src/importexport/capella/internal/capella.h
sed -n '680,715p' src/importexport/capella/internal/capella.h
echo "--- exact read signatures around Capella class boundaries ---"
python3 - <<'PY'
from pathlib import Path
p = Path('src/importexport/capella/internal/capella.h')
text = p.read_text().splitlines()
for i,line in enumerate(text,1):
if 'class CapellaReader' in line or 'class Capella' in line or 'bool read(' in line or 'Size readUnsigned' in line:
print(f"{i}: {line}")
for j in range(i+1, min(i+20, len(text)+1)):
print(f"{j}: {text[j-1]}")
print("--- capella.cpp readUnsigned nearby ---")
text = Path('src/importexport/capella/internal/capella.cpp').read_text().splitlines()
for i,line in enumerate(text,1):
if 'unsigned Capella::readUnsigned' in line:
for j in range(i-5, min(i+20, len(text)+1)):
print(f"{j}: {text[j-1]}")
PYRepository: musescore/MuseScore
Length of output: 8963
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- capella.cpp read() implementation ---"
rg -n "bool Capella::read|bool .*read\\(void\\* p, qint64 len|Capella::read\\(" src/importexport/capella/internal/capella.cpp -A 35
echo
echo "--- behavioral probe for readUnsigned payload-read validation ---"
python3 - <<'PY'
def read_unsigned(buf, pos, fmt=None):
# Mirrors src/importexport/capella/internal/capella.cpp Capella::readUnsigned
c = buf[pos]
if c == 254:
# read 2 bytes as unsigned short little-endian
if len(buf) < pos + 1 + 2:
return None, pos + 1
return int.from_bytes(buf[pos+1:pos+1+2], byteorder='little'), pos + 1 + 2
elif c == 255:
# read 4 bytes as unsigned
if len(buf) < pos + 1 + 4:
return None, pos + 1
return int.from_bytes(buf[pos+1:pos+1+4], byteorder='little'), pos + 1 + 4
else:
return c, pos + 1
cases = {
"single byte EOF before payload": bytes([100]),
"single byte enough bytes after for payload": bytes([100] + [0]*100),
"254 marker EOF before 2-byte length": bytes([254]),
"254 marker truncated length, no payload check": bytes([254, 0]),
"255 marker EOF before 4-byte length": bytes([255]),
"255 marker truncated length, no payload check": bytes([255, 0, 0, 0]),
}
for name, data in cases.items():
length, end = read_unsigned(data, 0)
has_len = length is not None and end <= len(data)
has_payload = has_len and len(data) >= end + length
print(f"{name}: length={length}, ended_at={end}, len_bytes_available_on_payload_boundary={len(data)-end}, has_payload={has_payload}")
PYRepository: musescore/MuseScore
Length of output: 3115
Reject truncated length prefixes before allocation.
readUnsigned() reads 2 or 4 bytes after 254/255, so an EOF during readUnsigned() throws before returning a near-maximum length. The allocation guard is therefore for malformed length values only; the real issue is that truncated length-prefix reads throw BAD_FORMAT without a length limit before every payload allocation. Bound size/len to one allocation-safety limit before both std::vector<char> and raw new char[] allocations.
📍 Affects 1 file
src/importexport/capella/internal/capella.cpp#L1489-L1496(this comment)src/importexport/capella/internal/capella.cpp#L2164-L2171
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/importexport/capella/internal/capella.cpp` around lines 1489 - 1496,
Apply one shared allocation-safety limit to the length values returned by
readUnsigned() before every payload allocation: validate size before the
std::vector<char> allocation in
src/importexport/capella/internal/capella.cpp:1489-1496 and validate len before
the raw new char[] allocation at
src/importexport/capella/internal/capella.cpp:2164-2171. Preserve BAD_FORMAT
handling while ensuring malformed or truncated length prefixes cannot result in
oversized allocations.
Comment on lines
+726
to
+729
| if (midiChannel < 0 || midiChannel >= static_cast<int>(channelDefaults.size())) { | ||
| LOGE() << "midiChannel " << midiChannel << " out of range 0-" << channelDefaults.size(); | ||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Clean up gp on both failed-read paths.
Both validations can return false, but importGTP returns before deleting gp at src/importexport/guitarpro/internal/importgtp.cpp Line [2908].
src/importexport/guitarpro/internal/importgtp-gp4.cpp#L726-L729: ensure the failedGuitarPro4::readpath releasesgp.src/importexport/guitarpro/internal/importgtp-gp5.cpp#L569-L572: ensure the failedGuitarPro5::readTrackspath releasesgp.
📍 Affects 2 files
src/importexport/guitarpro/internal/importgtp-gp4.cpp#L726-L729(this comment)src/importexport/guitarpro/internal/importgtp-gp5.cpp#L569-L572
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/importexport/guitarpro/internal/importgtp-gp4.cpp` around lines 726 -
729, The failed-read paths leak gp before returning false. In
src/importexport/guitarpro/internal/importgtp-gp4.cpp:726-729, release gp when
GuitarPro4::read validation fails; in
src/importexport/guitarpro/internal/importgtp-gp5.cpp:569-572, release gp when
GuitarPro5::readTracks validation fails, preserving the existing false returns.
| muse::String title, subtitle, artist, album, composer, poet; | ||
| muse::StringList comments; | ||
| GpTrack channelDefaults[GP_MAX_TRACK_NUMBER * 2]; | ||
| std::array<GpTrack, GP_MAX_TRACK_NUMBER* 2> channelDefaults; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 'midiChannel|channelDefaults\[midiChannel\]' \
src/importexport/guitarpro/internal/importgtp.cppRepository: musescore/MuseScore
Length of output: 4662
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== importgtp header relevant section =="
sed -n '300,330p' src/importexport/guitarpro/internal/importgtp.h
echo
echo "== importgtp read functions around channelDefaults =="
sed -n '1488,1585p' src/importexport/guitarpro/internal/importgtp.cpp
echo
sed -n '2238,2332p' src/importexport/guitarpro/internal/importgtp.cpp
echo
echo "== GP_MAX definitions =="
rg -n 'GP_MAX_TRACK_NUMBER|GP_MAX.*CHANNEL|GP_DEFAULT_PERCUSSION_CHANNEL|sizeof.*channelDefaults|channelDefaults' src/importexport/guitarpro/internalRepository: musescore/MuseScore
Length of output: 17522
Validate midiChannel before reading channelDefaults in the legacy readers.
channelDefaults has 64 entries, but GuitarPro2::read and GuitarPro3::read index it with readInt() - 1 without a >= 0 && < channelDefaults.size() guard, so malformed GuitarPro values can still read out of bounds. Apply the same guard used in GP4/GP5 readers before the patch and volume/pan/chorus/reverb reads.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/importexport/guitarpro/internal/importgtp.h` at line 324, Update the
legacy GuitarPro2::read and GuitarPro3::read paths to validate midiChannel with
a >= 0 and < channelDefaults.size() guard before indexing channelDefaults or
reading patch, volume, pan, chorus, and reverb values. Match the existing
validation behavior used by the GP4/GP5 readers, preserving normal processing
for valid channels.
mathesoncalum
approved these changes
Aug 4, 2026
Jojo-Schmitz
reviewed
Aug 4, 2026
| /* -7 -6 -5 -4 -3 -2 -1 0 1 2 3 4 5 6 7 */ | ||
| /* */ 7, 4, 1, 5, 2, 6, 3, 0, 4, 1, 5, 2, 6, 3, 0 | ||
| }; | ||
| if (int(key) + 7 >= keyOffsets.size()) { |
Contributor
There was a problem hiding this comment.
Seems here a cast (to int) is needed
if (int(key) + 7 >= int(keyOffsets.size())) {
miiizen
force-pushed
the
47ImporterFixes
branch
from
August 5, 2026 09:38
59f0dde to
6dcca8f
Compare
Jojo-Schmitz
reviewed
Aug 5, 2026
|
|
||
| void Capella::verifyLength(unsigned len) const | ||
| { | ||
| if (len < 0 || len > f->bytesAvailable()) { |
Contributor
There was a problem hiding this comment.
Here's a superfluous check of unsigned len being less than 0...
miiizen
force-pushed
the
47ImporterFixes
branch
from
August 5, 2026 14:27
6dcca8f to
ba07a10
Compare
mathesoncalum
approved these changes
Aug 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves: Collection of out of bounds & overflow errors in Guitar Pro, Capella and Band in a Box file importers