Uploaded image for project: 'Qt'
  1. Qt
  2. QTBUG-111538

QDockAreaLayout::updateSeparatorWidgets crash with null pointer dereference sometime after restoring state due to recursion into QMainWindowLayout::setGeometry causing corruption of separatorWidgets

    XMLWordPrintable

Details

    • Bug
    • Resolution: Fixed
    • P1: Critical
    • 6.5.1, 6.6.0
    • 6.4.2
    • Widgets: Layout
    • None
    • macOS
    • All
    • 5b8ffc6d2 (dev), fc9853dcb (dev), 4c8c29f55 (6.5), 355597f7d (6.5)

    Description

      The QDockAreaLayout::updateSeparatorWidgets function resizes the separatorWidgets list at the end, presumably to discard no-longer-used separators if the number of docks goes down. If this resize causes the separatorWidgets list to grow, though, it will be filled with null pointers. Then, when QDockAreaLayout::updateSeparatorWidgets is called again later, it will try to call raise on the null pointer and crash at that point.

      Interesting in reviewing the commit history is that this raise call was explicitly excluded by `#ifndef QT_MAC_USE_COCOA` in very old versions of Qt4, though this does not appear to have anything to do with the actual root cause of the problem.

      I expect this is related to QTBUG-44540 and possibly many other crasher bugs in this feature.

      To crash, do these things:

      1. Call QMainWindow::restoreState on a new QMainWindow with, at most, the following criteria:

      a. The window has Qt::WindowMaximized or Qt::WindowFullscreen state flags;
      b. The window has not been shown yet;
      c. The window does not fit on the screen, according to QMainWindowLayoutState::fits (n.b. this function is probably doing a wrong calculation since the saved state can come from a window that fit on the screen);
      d. The window has a tabified dock by having called tabifyDockWidget;
      e. The saved state has at least two docks (the tabified one, and one more one), resulting in two separator widgets being created in the final state.

      The first three criteria will cause state to not be immediately applied and instead go to QMainWindowLayout::restoredState along with a timer to actually restore once layout has settled; the fourth and fifth will cause corruption later in the restoration process.

      The first call to restore state will lead to a call to QMainWindowLayout::getTabBar if tabifyDockWidget was previously called (because usedTabBars will be populated, therefore this call will cause activate to be called, which will cause setGeometry to be called), which will create a layoutState with 1 separator widget. This state will be saved to restoredState. No separator will be created for the second dock because that part has not actually been restored from the saved state yet.

      2. Call QMainWindow::show to show the window. This will cause a correct layoutState to be created with 2 separator widgets, but this state will be overwritten by restoredState.
      3. A Resize event is triggered. This will cause a correct layoutState to be created with 2 separator widgets, but QMainWindowLayout::setGeometry will overwrite this state with restoredState.
      4. Call QApplication::exec to start the main event loop.
      5. A LayoutRequest event is triggered. This will cause a correct layoutState to be created with 2 separator widgets, but QMainWindowLayout::setGeometry will overwrite this state with restoredState.
      6. QDockAreaLayout::updateSeparatorWidgets calls QWidget::show on the second separator widget. This calls QMainWindowLayout::setGeometry, even though we just came from there. This overwrites the layoutState which contains the currently processing QDockAreaLayout with restoredState.
      7. Now that layoutState has been clobbered, the counter j in updateSeparatorWidgets is wrong.
      8. updateSeparatorWidgets calls separatorWidgets.resize(j), causing it to be zero-filled.
      9. Multiple additional LayoutRequest events are triggered. Repeat steps 5 through 8.
      10. The timer to erase restoredState fires and it is finally deleted. The state of the QDockAreaLayout is now corrupted, with a null pointer in separatorWidgets.
      11. Perform any action that causes another LayoutRequest. This will cause QMainWindowLayout to try to apply the state with the invalid separatorWidgets, which calls updateSeparatorWidgets, which tries to use the null separator widget, which crashes.

      So there are multiple problems in this code that are contributing to this crash, and probably also causing other issues with state restoration. At the least:

      1. Having a tabified dock should not cause restoration to occur prematurely; this causes an incomplete state to be written to restoredState
      2. QDockAreaLayout::updateSeparatorWidgets should not cause recursion into the parent setGeometry, and/or that recursion should not cause the state to be overwritten with an earlier copy
      3. QDockAreaLayout::updateSeparatorWidgets should probably have more defensive checks like its identical counterpart function in QDockAreaLayoutInfo so that if there is corruption (in the form of null pointers in the list) it does not cause a crash later (though the checks in QDockAreaLayoutInfo are also bad since it does not actually replace any null pointers with new valid ones, it just appends a new good one and then sizes down the list to truncate the new good one that was appended).
      4. QDockAreaLayout::updateSeparatorWidgets should never be resizing the list of separators to be larger than it was as this will cause it to insert null pointers.

      Attachments

        No reviews matched the request. Check your Options in the drop-down menu of this sections header.

        Activity

          People

            vhilshei Volker Hilsheimer
            csnover C S
            Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved:

              Gerrit Reviews