More importer fixes [4.7.5] - #34462
Conversation
|
@coderabbitai full review |
✅ Action performedFull review finished. |
📝 WalkthroughWalkthroughThe importers now add defensive validation for malformed input. PowerTab checks staff, string, and beat references. MIDI rejects empty SysEx payloads. OVE validates block sizes before stream creation and parsing. TableEdit validates text, string, measure, and UTF-8 data, and stops processing missing volta measures. 🚥 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}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/importexport/tabledit/internal/importtef.cpp (2)
772-809: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMove the index validation before allocating
staffTextto avoid a memory leak.
Factory::createStaffText(segment)at line 799 allocates theStaffTextobject before the bounds check at lines 800-803. WhentextMarker.indexis out of range,continueskips the object without ever callingsegment->add(staffText)or deleting it. The object is orphaned: it is never attached to the tree and never freed. Every malformedtextMarker.indexin a TEF file leaks oneStaffText.Other functions in this file (for example
addRestand the chord-creation code increateContents) validate first, then create and immediately add the item. Apply the same order here.🐛 Proposed fix to validate before allocating
- StaffText* staffText = Factory::createStaffText(segment); - if (textMarker.index < 0 || textMarker.index >= static_cast<int>(tefTexts.size())) { - LOGE() << "text marker index invalid"; - continue; - } + if (textMarker.index < 0 || textMarker.index >= static_cast<int>(tefTexts.size())) { + LOGE() << "text marker index invalid"; + continue; + } + StaffText* staffText = Factory::createStaffText(segment); muse::String text { tefTexts.at(textMarker.index).c_str() };🤖 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/tabledit/internal/importtef.cpp` around lines 772 - 809, Move the tefTextMarkers index bounds check in createTexts before the Factory::createStaffText(segment) allocation. Preserve the existing invalid-index log and continue behavior, and only create, configure, and add StaffText after the index is validated.
326-350: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the "TBD" placeholder with real diagnostic data.
The
breakat line 335 correctly avoids dereferencing a nullendMeasure. The log message at line 334 uses a hardcoded"TBD"string instead of identifying data, so the log gives no information about which ending or measure is affected.♻️ Proposed fix to include useful diagnostic data
- LOGD() << "Ending at " << "TBD" << " specifies non-existent end measure."; + LOGD() << "Ending " << ending.number << " at measure tick " << measure->tick().ticks() + << " specifies non-existent end measure.";🤖 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/tabledit/internal/importtef.cpp` around lines 326 - 350, Update the missing-next-measure diagnostic in addVolta to replace the hardcoded “TBD” with identifying data from the current ending and/or measure, such as ending.number and the measure tick or index. Preserve the existing break behavior when nextMeasure() returns null.
🤖 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/guitarpro/internal/importptb.cpp`:
- Around line 676-692: In the note-validation flow around trackStrings and k,
reject n.str when it is negative or outside trackStrings.size() before
calculating k. Preserve the existing tiedNotes and empty-track checks, and only
compute the string-data index for valid track string indices.
---
Outside diff comments:
In `@src/importexport/tabledit/internal/importtef.cpp`:
- Around line 772-809: Move the tefTextMarkers index bounds check in createTexts
before the Factory::createStaffText(segment) allocation. Preserve the existing
invalid-index log and continue behavior, and only create, configure, and add
StaffText after the index is validated.
- Around line 326-350: Update the missing-next-measure diagnostic in addVolta to
replace the hardcoded “TBD” with identifying data from the current ending and/or
measure, such as ending.number and the measure tick or index. Preserve the
existing break behavior when nextMeasure() returns null.
🪄 Autofix
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: f6435b09-c99f-4a1f-8b2a-aad76fe3462e
📒 Files selected for processing (4)
src/importexport/guitarpro/internal/importptb.cppsrc/importexport/midi/internal/midishared/midifile.cppsrc/importexport/ove/internal/ove.cppsrc/importexport/tabledit/internal/importtef.cpp
| if (n.str < 0 || static_cast<size_t>(n.str) >= tiedNotes.size()) { | ||
| LOGE() << "string index out of range, skipping note"; | ||
| continue; | ||
| } | ||
|
|
||
| const std::vector<int>& trackStrings = curTrack->infos[staff].strings; | ||
| if (trackStrings.empty()) { | ||
| LOGE() << "track has no strings, skipping note"; | ||
| continue; | ||
| } | ||
|
|
||
| const StringData* sd = score->staff(staff)->part()->instrument()->stringData(); | ||
| const int k = std::max(int(trackStrings.size()) - n.str - 1, 0); | ||
| if (!sd || static_cast<size_t>(k) >= sd->stringList().size()) { | ||
| LOGE() << "string data missing for note pitch"; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject string indices that exceed trackStrings.
Line 688 maps an invalid n.str to k == 0. A malformed note can then use the lowest string pitch instead of being skipped. Validate n.str against trackStrings.size() before calculating k.
Proposed fix
+ const std::vector<int>& trackStrings = curTrack->infos[staff].strings;
if (n.str < 0 || static_cast<size_t>(n.str) >= tiedNotes.size()) {
LOGE() << "string index out of range, skipping note";
continue;
}
- const std::vector<int>& trackStrings = curTrack->infos[staff].strings;
if (trackStrings.empty()) {
LOGE() << "track has no strings, skipping note";
continue;
}
+ if (static_cast<size_t>(n.str) >= trackStrings.size()) {
+ LOGE() << "string index exceeds track string count, skipping note";
+ continue;
+ }
const StringData* sd = score->staff(staff)->part()->instrument()->stringData();
- const int k = std::max(int(trackStrings.size()) - n.str - 1, 0);
+ const int k = int(trackStrings.size()) - n.str - 1;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (n.str < 0 || static_cast<size_t>(n.str) >= tiedNotes.size()) { | |
| LOGE() << "string index out of range, skipping note"; | |
| continue; | |
| } | |
| const std::vector<int>& trackStrings = curTrack->infos[staff].strings; | |
| if (trackStrings.empty()) { | |
| LOGE() << "track has no strings, skipping note"; | |
| continue; | |
| } | |
| const StringData* sd = score->staff(staff)->part()->instrument()->stringData(); | |
| const int k = std::max(int(trackStrings.size()) - n.str - 1, 0); | |
| if (!sd || static_cast<size_t>(k) >= sd->stringList().size()) { | |
| LOGE() << "string data missing for note pitch"; | |
| continue; | |
| } | |
| const std::vector<int>& trackStrings = curTrack->infos[staff].strings; | |
| if (n.str < 0 || static_cast<size_t>(n.str) >= tiedNotes.size()) { | |
| LOGE() << "string index out of range, skipping note"; | |
| continue; | |
| } | |
| if (trackStrings.empty()) { | |
| LOGE() << "track has no strings, skipping note"; | |
| continue; | |
| } | |
| if (static_cast<size_t>(n.str) >= trackStrings.size()) { | |
| LOGE() << "string index exceeds track string count, skipping note"; | |
| continue; | |
| } | |
| const StringData* sd = score->staff(staff)->part()->instrument()->stringData(); | |
| const int k = int(trackStrings.size()) - n.str - 1; | |
| if (!sd || static_cast<size_t>(k) >= sd->stringList().size()) { | |
| LOGE() << "string data missing for note pitch"; | |
| continue; | |
| } |
🤖 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/importptb.cpp` around lines 676 - 692, In
the note-validation flow around trackStrings and k, reject n.str when it is
negative or outside trackStrings.size() before calculating k. Preserve the
existing tiedNotes and empty-track checks, and only compute the string-data
index for valid track string indices.
569678b to
c31faf5
Compare
2 participants
Continuing in a similar vein to #34448, this time for Overture, Power Tab, TablEdit, and MIDI.