Fix bug where pressing home on empty tag field crashes the program. #11346
Conversation
| @@ -337,7 +337,7 @@ struct TagsEdit::Impl | |||
| assert(i < tags.size()); | |||
| auto occurrencesOfCurrentText = | |||
| std::count_if(tags.cbegin(), tags.cend(), [this](const auto& tag) { return tag.text == currentText(); }); | |||
| if (currentText().isEmpty() || occurrencesOfCurrentText > 1) { | |||
| if ((currentText().isEmpty() && tags.size() > 1) || occurrencesOfCurrentText > 1) { | |||
| tags.erase(std::next(tags.begin(), std::ptrdiff_t(editing_index))); | |||
There was a problem hiding this comment.
Yikes this line is ugly as sin. The line below this is what actually causes the crash because it decrements the editing index to -1. We also need to put guards in place in the currentText functions that blindly take the editing_index without any bounds checks.
There was a problem hiding this comment.
The change fixes the specific problem. The tags list will have at least size of 1, such that even after running the function, editing_index will be >=0 and the invariant hold.
Adding guards is probably a good idea, but I'd suggest to do some refactoring instead: What would be a good fallback for currentText() if editing_index is OoB? Crash gracefully? currentRect and currentText may return a reference.
There was a problem hiding this comment.
currentText should return an empty string and currentRect a zero sized rect. At the end of the day the variables in this class are not safe and the class needs to be refactored to make it safer.
There was a problem hiding this comment.
Given the current design of the class, I'd consider handling out-of-bounds as follows:
If tags is not empty, set editing_index to 0, then return tags[editing_index].
If tags is empty, add an empty tag, set editing_index to 0 and return.
Alternatively, we could introduce set/get currentText()/currentRect() member functions to avoid the reference problem (currentText() and currentRect() return references that are actually written to https://github.com/libklein/keepassxc/blob/develop/src/gui/tag/TagsEdit.cpp#L399. This would be my preferred option.
There was a problem hiding this comment.
That's just nuts, we should never be returning a reference to a QString. QString is optimized in the backend. If you are inside the class you don't/shouldn't generally use setter/getter functions. Those are meant for external users of the class.
Fix bug where pressing home on empty tag field crashes the program.
Bug is caused by https://github.com/libklein/keepassxc/blob/develop/src/gui/tag/TagsEdit.cpp#L340. The
currentText().isEmpty()allows to invalidate the invariant if the current (and only) tag is empty. In this case, tags is resized to 0 (the only tag is erased) and the program crashes ascurrentText()accessed in https://github.com/libklein/keepassxc/blob/develop/src/gui/tag/TagsEdit.cpp#L504 fails as the tags list is now empty (https://github.com/libklein/keepassxc/blob/develop/src/gui/tag/TagsEdit.cpp#L387).Fixes: #11344