mirror of
https://github.com/qelectrotech/qelectrotech-source-mirror.git
synced 2026-10-10 22:24:13 +02:00
Fix the double free when a deleted cable is undone and edited again
Deleting a cable and pressing Ctrl+Z twice leaves the cable out of the project while AddCableCommand still stands and still thinks it has to clean that cable up. The next edit of any kind makes the stack throw the undone commands away: ~RemoveCableCommand frees the cable, and ~AddCableCommand frees the same memory again. A plain build hides it, AddressSanitizer reports it as a heap-use-after-free in addcablecommand.cpp (blocking review comment by ispyisail). AddCableCommand now holds the cable as a QPointer, which is what RemoveCableCommand already did: whoever of the two runs first frees it, the other one sees a null pointer and has nothing left to do, in either order of destruction. The destructor also stopped dereferencing project() without checking it, which it did on that very line. Covered by tst_cableundointegration, a probe linked against the application's objects the way the other integration probes are. It draws a cable, deletes it, undoes twice and pushes a further edit -- the sequence which used to crash. Verified in both directions: with the raw pointer the probe dies with SIGSEGV inside ~AddCableCommand called from QUndoStack::push, with the QPointer it prints its PASS line.
This commit is contained in:
Kellermorph
@@ -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;
|
||||
|
||||
@@ -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<Cable> m_cable;
|
||||
///The line itself, owned by the folio once it is drawn
|
||||
QPointer<CablePart> m_part;
|
||||
QPointer<Diagram> m_diagram;
|
||||
|
||||
@@ -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 $<TARGET_FILE:qelectrotech>
|
||||
--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
|
||||
|
||||
@@ -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 <QApplication>
|
||||
#include <QFontDatabase>
|
||||
#include <QSettings>
|
||||
#include <QTemporaryDir>
|
||||
#include <QUuid>
|
||||
#include <cstdio>
|
||||
#include <stdexcept>
|
||||
|
||||
#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;
|
||||
}
|
||||
Executable
+60
@@ -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)
|
||||
Reference in New Issue
Block a user