View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0017965 | Scribus | Undo/Redo | public | 2026-09-05 03:09 | 2026-09-05 03:09 |
| Reporter | buttar | Assigned To | |||
| Priority | normal | Severity | minor | Reproducibility | N/A |
| Status | new | Resolution | open | ||
| Platform | Linux | OS | Fedora Workstation | OS Version | 44 |
| Product Version | 1.7.4.svn | ||||
| Summary | 0017965: [patch] Undo Delete Selection Hardening | ||||
| Description | - Applies after `undo-delete-textframes-with-marks-v1.00.patch` from 0017912 - Depends on that patch’s filtered `undoItems` selection snapshot, and hardens that snapshot with guarded pointers. ##Hardening - Converts the `DELETE_ITEM` undo selection snapshot from raw `PageItem-` pointers to `QPointer<PageItem>`. - Validates the referenced item before undo/redo dereferences it. - Restores selections using only still-live items. - Uses the validated current item for restore context instead of assuming the first stored entry is valid. ##Protection - Prevents stale `PageItem-` references in delete undo states from becoming dangling-pointer crashes. - Covers cases where a referenced item is destroyed after the undo snapshot is created but before undo/redo uses it. - Turns such failures into safely skippable null references rather than use-after-free behaviour. ##Relationship to the Direct Fix - The earlier patch fixes the known semantic bug by excluding generated auto note frames from the `DELETE_ITEM` snapshot. - This patch provides broader lifetime safety for the corrected snapshot. - It does not replace the direct fix; it protects the same undo path against future stale-reference cases. ##Scope - Limited to the `DELETE_ITEM` undo path. - Does not change `Selection`, marquee selection, note/mark handling, or the general undo architecture. - Does not globally convert other undo-state raw pointers to `QPointer`; those have different ownership/restoration semantics and remain out of scope. | ||||
| Tags | #please_test | ||||
| Attached Files | undo-delete-selection-hardening-v1.00.patch (4,736 bytes)
Index: scribus/scpage.cpp
===================================================================
--- scribus/scpage.cpp (revision 27814)
+++ scribus/scpage.cpp (working copy)
@@ -250,7 +250,7 @@
else if (ss->contains("CREATE_ITEM"))
restorePageItemCreation(dynamic_cast<ScItemState<PageItem*>*>(ss), isUndo);
else if (ss->contains("DELETE_ITEM"))
- restorePageItemDeletion(dynamic_cast<ScItemState< QList<PageItem*> >*>(ss), isUndo);
+ restorePageItemDeletion(dynamic_cast<ScItemState< QList<QPointer<PageItem>> >*>(ss), isUndo);
else if (ss->contains("CONVERT_ITEM"))
restorePageItemConversion(dynamic_cast<ScItemState<QPair<PageItem*, PageItem*> >*>(ss), isUndo);
else if (ss->contains("CONVERT_ITEM_TO_SYMBOL"))
@@ -392,39 +392,49 @@
m_Doc->m_Selection->delaySignalsOff();
}
-void ScPage::restorePageItemDeletion(ScItemState< QList<PageItem*> > *state, bool isUndo)
+void ScPage::restorePageItemDeletion(ScItemState< QList<QPointer<PageItem>> > *state, bool isUndo)
{
if (!state)
return;
int stateCode = state->transactionCode;
- QList<PageItem*> itemList = state->getItem();
+ QList<QPointer<PageItem>> itemList = state->getItem();
int id = state->getInt("ITEMID");
int id2 = state->getInt("ID");
- if (itemList.count() <= 0)
+ if (id2 < 0 || id2 >= itemList.count() || itemList.at(id2).isNull())
return;
+
+ PageItem* stateItem = itemList.at(id2).data();
m_Doc->view()->deselectItems(true);
bool oldMPMode = m_Doc->masterPageMode();
- m_Doc->setMasterPageMode(!itemList.at(0)->OnMasterPage.isEmpty());
+ m_Doc->setMasterPageMode(!stateItem->OnMasterPage.isEmpty());
if (m_Doc->appMode == modeEditClip) // switch off from edit shape
m_Doc->scMW()->nodePalette->EndEdit();
m_Doc->m_Selection->delaySignalsOn();
if (isUndo)
{
//CB #3373 reinsert at old position and renumber items
- PageItem* oldItem = itemList.at(id2);
+ PageItem* oldItem = stateItem;
if (oldItem->Parent && oldItem->Parent->isGroup())
oldItem->Parent->asGroupFrame()->groupItemList.insert(id, oldItem);
else
m_Doc->Items->insert(id, oldItem);
if (oldItem->isBookmark)
m_Doc->scMW()->AddBookMark(oldItem);
- m_Doc->m_Selection->addItems(itemList);
+
+ QList<PageItem*> restoredItems;
+ restoredItems.reserve(itemList.count());
+ for (const QPointer<PageItem>& item : std::as_const(itemList))
+ {
+ if (!item.isNull())
+ restoredItems.append(item.data());
+ }
+ m_Doc->m_Selection->addItems(restoredItems);
}
else
{
Selection tmpSelection(m_Doc, false);
- PageItem* ite = itemList.at(id2);
+ PageItem* ite = stateItem;
if (m_Doc->m_Selection->findItem(ite) != -1)
{
if (m_Doc->appMode == modeEdit || m_Doc->appMode == modeEditTable)
Index: scribus/scpage.h
===================================================================
--- scribus/scpage.h (revision 27814)
+++ scribus/scpage.h (working copy)
@@ -28,6 +28,7 @@
#include <QList>
#include <QPair>
+#include <QPointer>
#include <QRectF>
#include <QString>
@@ -114,7 +115,7 @@
protected:
UndoManager* const undoManager;
void restorePageItemCreation(ScItemState<PageItem*> *state, bool isUndo);
- void restorePageItemDeletion(ScItemState< QList<PageItem*> > *state, bool isUndo);
+ void restorePageItemDeletion(ScItemState< QList<QPointer<PageItem>> > *state, bool isUndo);
void restorePageAttributes(SimpleState *state, bool isUndo);
void restorePageItemConversion(ScItemState<QPair<PageItem*, PageItem*> >*state, bool isUndo);
void restorePageItemConversionToSymbol(ScItemState<QPair<PageItem*, PageItem*> >*state, bool isUndo);
Index: scribus/scribusdoc.cpp
===================================================================
--- scribus/scribusdoc.cpp (revision 27814)
+++ scribus/scribusdoc.cpp (working copy)
@@ -12048,7 +12048,7 @@
}
// Auto note frames are recreated by note undo and can be destroyed before item undo.
- QList<PageItem*> undoItems;
+ QList<QPointer<PageItem>> undoItems;
undoItems.reserve(delItems.count());
for (PageItem* item : delItems)
{
@@ -12116,11 +12116,11 @@
m_ScMW->DelBookMark(currItem);
if (UndoManager::undoEnabled() && (selectedItemCount > 0) && !forceDeletion)
{
- auto *is = new ScItemState< QList<PageItem*> >(Um::Delete + " " + currItem->getUName(), "", Um::IDelete);
+ auto *is = new ScItemState< QList<QPointer<PageItem>> >(Um::Delete + " " + currItem->getUName(), "", Um::IDelete);
is->setItem(undoItems);
is->set("DELETE_ITEM");
is->set("ITEMID", itemList->indexOf(currItem));
- is->set("ID", undoItems.indexOf(currItem));
+ is->set("ID", undoItems.indexOf(QPointer<PageItem>(currItem)));
m_undoManager->action(Pages->at(0), is, currItem->getUPixmap());
}
itemList->removeAll(currItem);
| ||||
| Patch | Yes | ||||
