gui: deprecated QCharRef usage in splashscreen #18080

1. 
   fanquake commented at 3:53 am on February 6, 2020: member

   Noticed today while building master.

   0GUI: 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.
   

   From a quick look the issue is somewhere in splashscreen.cpp.

   You should see the warning if you quit while the splashscreen is still displayed. Likely requires a recent version of Qt (I’m using 5.14.1).

   Want to work on this issue?

   The purpose of the good first issue label is to highlight which issues are suitable for a new contributor without a deep understanding of the codebase.

   You do not need to request permission to start working on this. You are encouraged to comment on the issue if you are planning to work on it. This will help other contributors monitor which issues are actively being addressed and is also an effective way to request assistance if and when you need it.

   For guidance on contributing, please read CONTRIBUTING.md before opening your pull request.

2. fanquake added the label GUI on Feb 6, 2020
3. fanquake added the label good first issue on Feb 6, 2020
4. 
   hebasto commented at 6:26 am on February 6, 2020: member

   QTBUG-80346 seems a bit similar.
5. 
   brakmic commented at 12:08 pm on February 9, 2020: contributor

   Hi,

   I found this bugfix from Nokia/Qt that dealt with the same problem: an index pointing outside the valid range of a QString.

   Then I checked for any index referencing in splashscreen.cpp and found only this one here: https://github.com/bitcoin/bitcoin/blob/master/src/qt/splashscreen.cpp#L140

   I know, it’s a bit amateur way of trying to find a bug, but pointing at index in a string could only mean something like this, if I am not mistaken. I have also searched for similar accesses like .at(index) but couldn’t find any.

   I will now try to reproduce the bug.

   Regards,

6. 
   brakmic commented at 1:59 pm on February 9, 2020: contributor

   According to Qt’s documentation the text() function could also return an empty string, which implies that one should check for them before indexing any chars within.

   https://doc.qt.io/qt-5/qkeyevent.html#text

   However, as I am still unable to reproduce the error mentioned by @fanquake I don’t know if I should open a PR for that.

   It would basically contain this:

   line 140, splashscreen.cpp

   0if(keyEvent->text().count() > 0 && keyEvent->text()[0] == 'q') {
   1    m_node.startShutdown();
   2}
   

7. 
   hebasto commented at 2:56 pm on February 9, 2020: member

   QCharRef QString::operator[](int position):

   Note: Before Qt 5.14 it was possible to use this operator to access a character at an out-of-bounds position in the string, and then assign to such a position, causing the string to be automatically resized. Furthermore, assigning a value to the returned QCharRef would cause a detach of the string, even if the string has been copied in the meanwhile (and the QCharRef kept alive while the copy was taken). These behaviors are deprecated, and will be changed in a future version of Qt.

8. 
   hebasto commented at 4:27 pm on February 9, 2020: member

   @fanquake

   You should see the warning if you quit while the splashscreen is still displayed. Likely requires a recent version of Qt (I’m using 5.14.1).

   Using Qt 5.14.0 on bionic and 5.14.1 on macOS 10.14. Couldn’t reproduce the issue. Update: #18080 (comment)

9. 
   brakmic commented at 4:28 pm on February 9, 2020: contributor

   I’m using Qt 5.14.1 on macOS Catalina 10.14.3, but no warning to see.
10. 
    hebasto commented at 5:08 pm on February 9, 2020: member

    Actually a QCharRef warning is fired if any modifier key is pressed while the splashscreen is still displayed.
11. 
    brakmic commented at 5:36 pm on February 9, 2020: contributor

    Actually a QCharRef warning is fired if any modifier key is pressed while the splashscreen is still displayed.

    Thanks for the hint!

    Yes, this is true. Just checked it. The small change in the if-statement seems to prevent it from happening. Will then open new PR.

12. 
    hebasto commented at 5:38 pm on February 9, 2020: member

    Actually a QCharRef warning is fired if any modifier key is pressed while the splashscreen is still displayed.

    Thanks for the hint!

    Yes, this is true. Just checked it. The small change in the if-statement seems to prevent it from happening. Will then open new PR.

    Please review #18101.

13. 
    brakmic commented at 5:38 pm on February 9, 2020: contributor

    Actually a QCharRef warning is fired if any modifier key is pressed while the splashscreen is still displayed.

    Thanks for the hint! Yes, this is true. Just checked it. The small change in the if-statement seems to prevent it from happening. Will then open new PR.

    Please review #18101.

    Ah, OK, will do it! Thanks.

14. laanwj closed this on Feb 10, 2020

    ---

15. PastaPastaPasta referenced this in commit 4efc842eba on Jun 27, 2021
16. PastaPastaPasta referenced this in commit 06a08d3cb5 on Jun 28, 2021
17. PastaPastaPasta referenced this in commit 5b21767e40 on Jun 29, 2021
18. PastaPastaPasta referenced this in commit dfddbc81df on Jul 1, 2021
19. PastaPastaPasta referenced this in commit 5cfdaa18d5 on Jul 1, 2021
20. PastaPastaPasta referenced this in commit f2cd8c9da0 on Jul 14, 2021
21. PastaPastaPasta referenced this in commit c90a5fddb4 on Jul 14, 2021
22. PastaPastaPasta referenced this in commit 2a3fa63dc1 on Jul 15, 2021
23. DrahtBot locked this on Feb 15, 2022

Contributors
fanquake hebasto brakmic

Labels
GUI good first issue