Add Lua APIs to check if there are pending transactions, clean up tests#337
Open
jbms wants to merge 2 commits into
Open
Add Lua APIs to check if there are pending transactions, clean up tests#337jbms wants to merge 2 commits into
jbms wants to merge 2 commits into
Conversation
Owner
|
No, I don't want people to control the compositor from Lua. The idea of Lua is to write simple scripts, not provide a framework to run tests on the compositor. Also, I don't want the test suite to become an ends in itself, and we are slowly getting there. More often than not, tests are good, but they quickly become obsolete when a bug is fixed, and regressions are not even easy to find using the old tests because they usually happen because of new code, so in the end you have a huge test suite where finding things is hard, and it only proves lots of old bugs were fixed. I prefer to document the issues and explain why they happened than to have a myriad of tests that only prove a bug was fixed. |
jbms
force-pushed
the
lua-animation-stepping
branch
5 times, most recently
from
af3dfa2 to
d25d59e
Compare
Contributor
Author
|
Okay, I removed the Lua apis for animation stepping and querying animation duration. I retained only the apis for checking if animation is progress and if there are pending transactions. I also removed the tests that relied on animation stepping. I explored whether it was possible to implement the animation tests non-intrusively using libfaketime or a similar approach but it seemed to require a lot of complexity so I decided to just remove the animation testing altogether. |
jbms
changed the title
jbms
force-pushed
the
lua-animation-stepping
branch
4 times, most recently
from
0961a5a to
f5b9dd7
Compare
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 a series of workspace switches is
executed in a single transaction, leaving intermediate workspaces empty and
unfocused so they are garbage collected at the transaction commit), these
references become dangling. Upon subsequent animation steps or completion of
the animation, accessing the workspaces triggers a use-after-free crash.
To address this, we register wl_listener callbacks on the node destroy signals
of the source and target workspaces. If either workspace is destroyed while the
animation is active, we null out the corresponding reference. All callback and
filter code paths are updated to safely handle null pointers.
Here is a minimal shell script to reproduce the crash on a build without this fix:
```bash
#!/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
```
Exposes scroll.animating() and scroll.pending_transactions() to Lua to allow external scripts and tests to query whether the compositor has settled. Also includes testing infrastructure updates (pytest.ini config, LSan suppressions, and helper methods in test_utils.py).
jbms
force-pushed
the
lua-animation-stepping
branch
from
f5b9dd7 to
e04ab7b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This adds the following Lua APIs:
scroll.animating(): Returns true if animation is in progress.scroll.pending_transactions(): Returns true if there are uncommitted changes.These APIs allow tests to verify that all changes have been applied.
This also updates the Python tests to run faster and more robustly.
Disable animations by default in the test compositor configuration to avoid unnecessary transaction delays.
Refactor existing integration tests to eliminate
time.sleep:tests/test_normal_exit.py: Usesubprocess.Popen.waitinstead of a manual polling loop.tests/test_workspace_split_uaf.py: Use the shared compositor (since animations are now disabled by default).wait_for_idleorwait_for_client_mapto wait for transactions to settle.Implement a thorough reset mechanism for the session-scoped
scroll_compositorfixture:ScrollInstance.reset()after each test.reset()useskill allto close all views, unplugs extra outputs, reloads the sway configuration to reset defaults (which also recreates the Lua state and discards registered callbacks), and recreates the default workspace 1 to reset its layout modifiers.Optimize test suite execution speed:
pytest-xdistto the Nix development shell.pytest.inito run tests in parallel with 4 workers by default (-n 4), which is stable under ASan and avoids OOM/thrashing.-Dbuildtype=debugoptimizedfor-O2) when building the compositor for tests under ASan.