diff --git a/sources/cable/addcablecommand.cpp b/sources/cable/addcablecommand.cpp index b7ad72c38..d3d81b41b 100644 --- a/sources/cable/addcablecommand.cpp +++ b/sources/cable/addcablecommand.cpp @@ -87,11 +87,19 @@ AddCableCommand::AddCableCommand(Cable *cable, Whatever the stack leaves behind has to be cleaned up here: a cable which is not held by the project any more belongs to this command, and so does a line which never made it onto a folio. + + The cable is a QPointer, so if another command took it away first it + is already null here and there is nothing left to do -- which is + exactly the point, since following a raw pointer here meant freeing + the same cable a second time. */ AddCableCommand::~AddCableCommand() { - if (m_cable && m_own_cable && !project()->cables().contains(m_cable)) { - delete m_cable; + if (m_cable && m_own_cable) { + QETProject *proj = project(); + if (!proj || !proj->cables().contains(m_cable.data())) { + delete m_cable.data(); + } } if (m_part && !m_part->scene()) { delete m_part; diff --git a/sources/cable/addcablecommand.h b/sources/cable/addcablecommand.h index bb4785225..2980ece12 100644 --- a/sources/cable/addcablecommand.h +++ b/sources/cable/addcablecommand.h @@ -55,8 +55,17 @@ class AddCableCommand : public QUndoCommand QETProject *project() const; private: - ///The cable the line belongs to - Cable *m_cable = nullptr; + /** + The cable the line belongs to. + + A pointer, not a raw one: RemoveCableCommand deletes the + cable when it goes, and both of them are torn down together + when the stack takes a new command. Whoever of the two runs + first frees the cable, so the other one must not follow it + into the freed memory -- QPointer goes null instead, and + this command then simply has nothing left to clean up. + */ + QPointer m_cable; ///The line itself, owned by the folio once it is drawn QPointer m_part; QPointer m_diagram; diff --git a/tests/qttest/CMakeLists.txt b/tests/qttest/CMakeLists.txt index 3a16bb45d..39799e9fe 100644 --- a/tests/qttest/CMakeLists.txt +++ b/tests/qttest/CMakeLists.txt @@ -87,6 +87,15 @@ if(Python3_Interpreter_FOUND AND CMAKE_GENERATOR STREQUAL "Ninja") --source ${CMAKE_CURRENT_SOURCE_DIR}/attached_plain_paste_probe.cpp --fixture ${CMAKE_CURRENT_SOURCE_DIR}/fixtures/unlinked_contact_label.qet) set_tests_properties(tst_attachedplainpaste PROPERTIES TIMEOUT 300 RUN_SERIAL TRUE) + # Deleting a cable, undoing the delete twice and then editing anything + # has both cable commands tearing down the same cable. Runs under the + # same objects the application uses, so a second free is caught. + add_test(NAME tst_cableundointegration + COMMAND ${Python3_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/run_cable_undo_probe.py + --build ${CMAKE_BINARY_DIR} --binary $ + --source ${CMAKE_CURRENT_SOURCE_DIR}/cable_undo_probe.cpp + --fixture ${CMAKE_CURRENT_SOURCE_DIR}/fixtures/unlinked_contact_label.qet) + set_tests_properties(tst_cableundointegration PROPERTIES TIMEOUT 300 RUN_SERIAL TRUE) endif() if(Python3_Interpreter_FOUND AND CMAKE_GENERATOR STREQUAL "Ninja") add_test(NAME tst_darkimageintegration diff --git a/tests/qttest/cable_undo_probe.cpp b/tests/qttest/cable_undo_probe.cpp new file mode 100644 index 000000000..b02cc06df --- /dev/null +++ b/tests/qttest/cable_undo_probe.cpp @@ -0,0 +1,105 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +// Cable undo ownership regression, linked against the application's objects. +// +// Deleting a cable and undoing the delete twice leaves the cable out of +// the project while AddCableCommand still holds it, and the next edit +// makes the stack throw both commands away. Whoever of the two runs +// first frees the cable; the other one must not follow it into the freed +// memory. A raw pointer there meant freeing the same cable twice, which +// a normal build hides and AddressSanitizer reports as a heap +// use-after-free in ~AddCableCommand. +#include +#include +#include +#include +#include +#include +#include + +#include "../../sources/diagram.h" +#include "../../sources/qetproject.h" +#include "../../sources/qetmessagebox.h" +#include "../../sources/cable/addcablecommand.h" +#include "../../sources/cable/cable.h" +#include "../../sources/cable/cablepart.h" +#include "../../sources/cable/editcablecommand.h" + +static void check(bool value, const char *message) +{ + if (!value) throw std::runtime_error(message); +} + +// Any edit the user makes afterwards: pushing it is what makes the stack +// discard the commands it still holds undone, and that is where the two +// destructors meet the same cable. +struct AnyEdit : QUndoCommand +{ + void redo() override {} + void undo() override {} +}; + +int main(int argc, char **argv) +{ + QApplication app(argc, argv); + std::freopen(argv[2], "w", stdout); + + QTemporaryDir settings; + QSettings::setDefaultFormat(QSettings::IniFormat); + QSettings::setPath(QSettings::IniFormat, QSettings::UserScope, settings.path()); + QCoreApplication::setOrganizationName("QETCableUndoRegression"); + QETProject::setBackupEnabled(false); + QET::QetMessageBox::setNonInteractive(true); + QFontDatabase::addApplicationFont(":/fonts/LiberationSans-Regular.ttf"); + + try + { + QETProject project(QString::fromLocal8Bit(argv[1])); + check(project.state() == QETProject::Ok, "fixture opens"); + Diagram *scene = project.diagrams().first(); + + // The state the cable tool leaves behind when it has drawn one + // line and the dialog accepted it: a cable holding the section, + // a line on the folio pointing back at that cable. + CablePartData data; + data.uuid = QUuid::createUuid(); + data.diagram = scene->uuid(); + data.p1 = QPointF(100, 100); + data.p2 = QPointF(300, 100); + + //The cable the create dialog hands over: owned by the project, + //which is the only way it may enter the project's list at all. + auto *cable = project.newCable(); + cable->addPart(data); + auto *item = new CablePart(data); + item->setCable(cable); + scene->addItem(item); + + // Draw the line. + scene->undoStack().push(new AddCableCommand(cable, item, scene, true)); + check(project.cables().contains(cable), "drawn cable is in the project"); + + // Select all and delete. + scene->undoStack().push(new RemoveCableCommand(scene, {item})); + check(!project.cables().contains(cable), "delete took the cable out of the project"); + + // Ctrl+Z, Ctrl+Z: the cable is in nobody's hands now, but both + // commands still stand and both think they may have to clean it up. + scene->undoStack().undo(); + scene->undoStack().undo(); + check(!project.cables().contains(cable), "cable orphaned by two undos"); + + // The next edit of any kind: the stack throws the undone commands + // away, and ~RemoveCableCommand frees the cable before + // ~AddCableCommand gets to look at it. + scene->undoStack().push(new AnyEdit); + + check(project.state() == QETProject::Ok, "project survives the edit"); + std::puts("PASS: deleting a cable, undoing twice and editing again frees it exactly once"); + } + catch (const std::exception &error) + { + std::printf("FAIL: %s\n", error.what()); + return 1; + } + return 0; +} diff --git a/tests/qttest/run_cable_undo_probe.py b/tests/qttest/run_cable_undo_probe.py new file mode 100755 index 000000000..7613ec164 --- /dev/null +++ b/tests/qttest/run_cable_undo_probe.py @@ -0,0 +1,60 @@ +#!/usr/bin/env python3 +"""Link the cable undo integration probe using an existing Ninja app build. + +Reuses app objects without duplicating the app or introducing a core-library +refactor. No application source or existing executable is replaced. The +generated manifest, probe binary, reports and export fixtures stay in build/. +""" +import argparse +import pathlib +import re +import shutil +import subprocess +import sys + +parser = argparse.ArgumentParser() +parser.add_argument('--build', required=True) +parser.add_argument('--binary', required=True) +parser.add_argument('--source', required=True) +parser.add_argument('--fixture', required=True) +args = parser.parse_args() +build = pathlib.Path(args.build).resolve() +binary = pathlib.Path(args.binary).resolve() +source = pathlib.Path(args.source).resolve() +fixture = pathlib.Path(args.fixture).resolve() +manifest = (build / 'build.ninja').read_text(encoding='utf-8') +match = re.search(r'^build (\S*?/sources/main\.cpp\.(?:obj|o)):', manifest, re.MULTILINE) +if not match: + sys.exit('Cannot find the application main object in the Ninja build.') +main_object = match.group(1) +probe_object = main_object.replace('main.cpp.', 'cable_undo_probe.cpp.') +probe_binary = binary.with_name('cable_undo_probe' + binary.suffix) +def escape(path): + return pathlib.Path(path).as_posix().replace('$', '$$').replace(':', '$:').replace(' ', '$ ') +main_source = source.parents[2] / 'sources' / 'main.cpp' +manifest = manifest.replace(main_object, probe_object) +manifest = manifest.replace(escape(main_source), escape(source)) +if binary.suffix: + manifest = manifest.replace(binary.name, probe_binary.name) +else: + old_target = escape(binary.name if binary.parent == build else binary) + new_target = escape(probe_binary.name if probe_binary.parent == build else probe_binary) + manifest = manifest.replace('build ' + old_target + ':', 'build ' + new_target + ':') + old_file = binary.name if binary.parent == build else binary.as_posix() + new_file = probe_binary.name if probe_binary.parent == build else probe_binary.as_posix() + manifest = re.sub(r'^ TARGET_FILE = ' + re.escape(old_file) + r'$', + ' TARGET_FILE = ' + new_file, manifest, flags=re.MULTILINE) +probe_manifest = build / 'cable-undo-probe.ninja' +probe_manifest.write_text(manifest, encoding='utf-8') +ninja = shutil.which('ninja') +if not ninja: + sys.exit('Ninja is required for the integration probe.') +target = probe_binary.name if probe_binary.parent == build else str(probe_binary) +subprocess.run([ninja, '-C', str(build), '-f', probe_manifest.name, target], check=True) +output = build / 'cable-undo' +output.mkdir(exist_ok=True) +report = output / 'results.txt' +result = subprocess.run([str(probe_binary), str(fixture), str(report), str(output)], timeout=90) +if report.exists(): + print(report.read_text(encoding='utf-8', errors='replace')) +sys.exit(result.returncode)