Cancel pending renaming action for a screen in dtor of ScreensPanel

[BenBE]

Fixes an UAF issue when editing the screen configuration.

Fixes: #1760

[Explorer09]

I tested this in macOS Tahoe just now and it seems like the bug fix works.

One issue remaining, though. When built in GCC with LTO there is this warning:

ScreensPanel.c:62:19: error: potential null pointer dereference [-Werror=null-dereference]
   62 |       item->value = this->saved;
      |                   ^

So the code has to tune a little bit:

diff --git a/ScreensPanel.c b/ScreensPanel.c
index eaf06920..e5ab1ded 100644
--- a/ScreensPanel.c
+++ b/ScreensPanel.c
@@ -57,10 +57,10 @@ static void ScreensPanel_delete(Object* object) {
    /* cancel any pending edit action */
    if (this->renamingItem) {
       ListItem* item = (ListItem*) Panel_getSelected(super);
-      assert(item);
       assert(item == this->renamingItem);
 
-      item->value = this->saved;
+      if (item)
+         item->value = this->saved;
       this->renamingItem = NULL;
       super->cursorOn = false;

And that's it.

[BenBE]

@Explorer09 Do you have an idea for a fix to the issue you mentioned in #1848? I think the fixes will likely be related thus I'd like to handle it in one go.

[Explorer09]

@Explorer09 Do you have an idea for a fix to the issue you mentioned in #1848? I think the fixes will likely be related thus I'd like to handle it in one go.

Nope. Not yet. I'm busy with my personal work these days and won't be able to look at the problem.

[Explorer09]

@Explorer09 Do you have an idea for a fix to the issue you mentioned in #1848? I think the fixes will likely be related thus I'd like to handle it in one go.

@BenBE

I just managed to get some free time and diagnosed the problem with GDB, and I have a partial fix:

diff --git a/ScreensPanel.c b/ScreensPanel.c
index 2ba7b940..acda42ad 100644
--- a/ScreensPanel.c
+++ b/ScreensPanel.c
@@ -77,6 +77,13 @@ static HandlerResult ScreensPanel_eventHandlerRenaming(Panel* super, int ch) {
    }
 
    switch (ch) {
+      case EVENT_SET_SELECTED: {
+         ListItem* item = (ListItem*) Panel_getSelected(super);
+         if (item != this->renamingItem)
+            goto renameFinish;
+         break;
+      }
+
       case 127:
       case KEY_BACKSPACE:
          if (this->cursor > 0) {
@@ -93,8 +100,9 @@ static HandlerResult ScreensPanel_eventHandlerRenaming(Panel* super, int ch) {
          if (!item)
             break;
          assert(item == this->renamingItem);
+renameFinish:
          free(this->saved);
-         item->value = xStrdup(this->buffer);
+         this->renamingItem->value = xStrdup(this->buffer);
          this->renamingItem = NULL;
          super->cursorOn = false;
          Panel_setSelectionColor(super, PANEL_SELECTION_FOCUS);

This at the minimum, avoids the crash when combined with your commit dbdad20. The issue though, is that the modified screen name might not get applied when the ScreensPanel itself loses focus.

In ScreenManager_run, it looks like the Panel_eventHandler function is fired only for panelFocus, so I didn't yet find a way to catch the event when the ScreensPanel loses focus.