Fix use-after-free crash during workspace switch animation

[jbms]

During workspace switch animations, references to the source and target workspaces (data->from and data->to) are tracked. If a workspace is destroyed while the animation is active (e.g., when it is emptied or its output is unplugged), these references become dangling. Upon completion or cancellation of the animation, accessing the workspaces triggers a use-after-free crash.

To address this, we avoid starting the switch animation if either the
source or target workspace is already marked for destruction (which is
when they can be freed).

Here is a minimal shell script to reproduce the crash on a build without this fix:

#!/bin/bash
set -x

# Setup isolated run environment
TEMP_DIR=$(mktemp -d)
export XDG_RUNTIME_DIR="$TEMP_DIR"
export WLR_BACKENDS=headless
export WLR_LIBINPUT_NO_DEVICES=1
export LSAN_OPTIONS=detect_leaks=0
unset DISPLAY
unset WAYLAND_DISPLAY

# Create minimal configuration
cat <<EOF > "$TEMP_DIR/config"
animations enabled on
EOF

# Start scroll compositor headlessly
./build/sway/scroll -c "$TEMP_DIR/config" -d > "$TEMP_DIR/scroll.log" 2>&1 &
SCROLL_PID=$!

# Wait for the compositor to start up and open its wayland socket
for i in {1..50}; do
    SOCKET_PATH=$(find "$XDG_RUNTIME_DIR" -name "wayland-*" | head -n 1)
    if [ -n "$SOCKET_PATH" ] && [ -S "$SOCKET_PATH" ]; then
        break
    fi
    sleep 0.1
done

if [ -z "$SOCKET_PATH" ] || [ ! -S "$SOCKET_PATH" ]; then
    echo "Error: scroll failed to start or open socket"
    cat "$TEMP_DIR/scroll.log"
    kill $SCROLL_PID || true
    rm -rf "$TEMP_DIR"
    exit 1
fi

SOCKET_NAME=$(basename "$SOCKET_PATH")

# Switch to workspace 2 and immediately switch back to workspace 1 to trigger UAF
export SWAYSOCK="$XDG_RUNTIME_DIR/i3-ipc.sock"
if [ ! -S "$SWAYSOCK" ]; then
    # Look for the IPC socket
    SWAYSOCK=$(find "$XDG_RUNTIME_DIR" -name "*.sock" | head -n 1)
fi

echo "Using SWAYSOCK=$SWAYSOCK"
./build/swaymsg/scrollmsg -s "$SWAYSOCK" "workspace 2; workspace 1" 2>&1

# Wait a brief moment for the crash to complete and the process to terminate
sleep 0.5

# Check if the compositor crashed
if kill -0 $SCROLL_PID 2>/dev/null; then
    echo "Compositor is still running! Reproduction FAILED."
    echo "=== COMPOSITOR LOG ==="
    cat "$TEMP_DIR/scroll.log"
    kill $SCROLL_PID
    rm -rf "$TEMP_DIR"
    exit 1
else
    echo "Compositor has terminated (expected)!"
    cat "$TEMP_DIR/scroll.log"
    rm -rf "$TEMP_DIR"
    exit 0
fi

[jbms]

I was able to reproduce the workspace animation use-after-free bug after all, and created this PR with a better fix.

[dawsers]

OK, this test is kind of cheating :-)

The to workspace should always exist, workspace_switch() should never receive a NULL workspace to switch to. That used to simplify the original code, and can also simplify this one.

[jbms]

Okay, now instead of tracking the node ids, this PR just avoids starting the animation if either workspace is marked for destruction.

[dawsers]

A command such as the one that causes the crash (workspace 3; workspace1) should be used with a lot of care regardless of this crash, and it doesn't make sense from a user point of view.

When executing several commands separated by ;, they are all part of the same transaction, and the transaction is committed after calling all the commands. In this case, the second animation setup will overwrite the first one's, because there will be no call to animation_callback_end until after the transaction, and both will be called then.

You can see that even if you add a container to each workspace to avoid the crash, you will end up in an invalid state.

Then, even with the fix, make the command more complicated, and you still crash scroll:

bindsym $mod+q workspace 2; workspace 3; workspace 1

1. Create a container in workspace 1
2. Switch to workspace 2 and create another container
3. Press $mod+q. It will access invalid memory.

I have a fix for this, but it is always possible to crash the compositor crafting commands like these. I am more worried about crashes that can be caused by general use of the program. In reality, nobody needs to change workspaces three times within the same transaction, if they do, it is because they are trying to crash the compositor, and I could argue at that point exit can do the same.

[jbms]

Certainly crashes that occur though normal use are more of a concern but I'd still argue that any ability to crash though commands or lua should be considered a bug. If there is some significant benefit to exposing "unsafe" functionality then it could be exposed but explicitly documented as unsafe, but I think it is fairly unlikely that there is any functionality in scroll that can't be made safe without significant overhead.

[jbms]

Okay, I have updated this to properly handle workspace 2; workspace 3; workspace 1 --- I am pretty sure the previous fix of storing node ids would have also worked, but the simplified version that just skipped animating if the from or to workspace was already marked for destruction was not sufficient.

[jbms]

If you point me at specific cases where you think it is still possible to crash the compositor using commands or the lua api, I can try to fix them also.