Deprecated and possibly unsafe string handling in chordlist.cpp
Reported version
3.5
Type
Development
Frequency
Once
Severity
S5 - Suggestion
Reproducibility
Always
Status
closed
Regression
No
Workaround
No
Project
Since upgrading to Qt 15.5.0 MuseScore's libmscore/chordlist.cpp produces many warnings:
unknown:unknown: Using QCharRef with an index pointing outside the valid range of a QString. The corresponding behavior is deprecated, and will be changed in a future version of Qt.
These are all caused by ParsedChord::parse(), in code that looks like:
QString tok = "";
for(i =0;...;++i)
tok[i] = ...
This may access memory beyond the string's character buffer and could lead to crashes. I suspect the tok[i] assignment can easily be replaced by tok.append(...), which is safe.
This was found in the current 3.x version, but git blame shows the code has been unchanged for years.
Fix version
3.6.0
Comments
a) Update to Qt 5.15.1
b) Wait another day or two and update to Qt 5.15.2
c) Which compiler/ environment is giving you that? I'm using 5.15.1 too, and also for 3.x meanwhile, and don't see any compiler warning, not in MSVC nor in QtCreator/MinGW
d) 3.x will continue to use Qt 5.9, so that deprecation warning can get savely ignored there. Not for master though
e) I've seen some similar warnings, from Clang-Tidy+Clazy, while playing with those, see also my https://github.com/musescore/MuseScore/pull/6841, these actually asked to change from
QChartoQCharRefin several places?Edit: I confused that with
QString::mid()vs.QString::midRef()And I see the these are not compiler warnings, but rather runtime warnings?!?
Warning: Using QCharRef with an index pointing outside the valid range of a QString. The corresponding behavior is deprecated, and will be changed in a future version of Qt.
Still with:
macOS High Sierra (10.13.6)
Xcode 10.1
Apple LLVM version 10.0.0 (clang-1000.11.45.5)
Qt 5.15.0
I suspect 3.x is relatively safe, but the code is basically an "array out of bounds" condition. It may work today, but it may also break tomorrow. Furthermore, we may find a Qt upgrade blocked in future unless this is solved.
It sure needs fixing, one way or another.
This was some of the very first code I wrote for MuseScore. At the time, I was brand new to Qt but it seemed to me this was something QString was supposed to do automatically. I guess that is what is deprecated now? Presumably indeed appending would work.
Hi Marc, don't worry about it, these things happen. Furthermore, the Qt documentation doesn't mention this in any detail. Normally the safest bet is to assume that operator does not allocate memory and you are not allowed to write outside the underlying buffer.
As I am not familiar with this part of the code, I am reluctant to suggest a better solution in more detail.
Well, an alternative that would be safe would be to pre-allocated the string, filled with zeroes, that's how I might have done it in C anyhow. But I think append makes more sense. I don't have 5.15 installed to test this though and am a bit afraid to mess up a working build environment right now. I could make the change if you like but I'd want someone else to check whether it makes the warnings go away. I also figure it's likely I'd miss a case or two.
Either appending character by character or determining which part of the string you want to copy and appending the relevant substring seems way to go (proper C++ idiom). I can verify whether your update fixes the warnings if you like.
Seems solved in 3.x by commit 5a03ffe.
or 5a03ffe
Oh nice, I had forgotten about this...
Sound like it's fixed then.
Automatically closed -- issue fixed for 2 weeks with no activity.