From 0229438f09ad2e95f3afc72d9defc02ea449b52f Mon Sep 17 00:00:00 2001 From: "Enrico Weigelt, metux IT consult" Date: Thu, 2 Jul 2026 14:33:25 +0200 Subject: [PATCH] ci: run pyxtest under valgrind memcheck on Ubuntu Add a dedicated xserver-build-ubuntu-valgrind job that builds a debug Xvfb and runs the pyxtest suite (test/pyxtest/) with --valgrind, so memory errors (invalid reads/writes, use-after-free, uninitialised value use) are caught in CI, not just OOB bugs ASAN happens to catch locally. The pytest-based harness (test/pyxtest/conftest.py, xserver.py, valgrind.py) already implements valgrind support; it was simply never wired into CI. pytest itself was also not installed in the Ubuntu CI image, so the existing pyxtest suite has been silently skipped (not built, since meson's find_program('pytest', required: false) found nothing) on every other Ubuntu lane too. Installing python3-pytest fixes that as a side effect: the normal xserver-build-ubuntu* jobs now also actually run pyxtest (without valgrind) as part of "meson test", exercising the existing protocol-fuzzing regression tests that were previously dead weight. Kept as a separate lane rather than adding --valgrind to the existing run-xserver-build-and-test.sh: valgrind is slow, and pyxtest only needs Xvfb, not the full Xorg/Xephyr/Xnest/XTS/piglit build the other lanes carry. Signed-off-by: Enrico Weigelt, metux IT consult --- .github/scripts/ubuntu/install-pkg.sh | 4 + .../scripts/ubuntu/run-pyxtest-valgrind.sh | 78 +++++++++++++++++++ .github/workflows/build-xserver.yml | 51 ++++++++++++ test/pyxtest/README.md | 3 + test/pyxtest/conftest.py | 69 ++++++++++++++++ test/pyxtest/xserver.py | 11 +++ 6 files changed, 216 insertions(+) create mode 100755 .github/scripts/ubuntu/run-pyxtest-valgrind.sh diff --git a/.github/scripts/ubuntu/install-pkg.sh b/.github/scripts/ubuntu/install-pkg.sh index abdfe4f76b..ebe44d1877 100755 --- a/.github/scripts/ubuntu/install-pkg.sh +++ b/.github/scripts/ubuntu/install-pkg.sh @@ -7,6 +7,10 @@ EPHEMERAL=" libexpat-dev libgles2-mesa-dev libxkbcommon-dev + python3-pytest + python3-pytest-timeout + python3-pytest-xdist + valgrind x11-utils x11-xserver-utils xauth diff --git a/.github/scripts/ubuntu/run-pyxtest-valgrind.sh b/.github/scripts/ubuntu/run-pyxtest-valgrind.sh new file mode 100755 index 0000000000..eded353f9f --- /dev/null +++ b/.github/scripts/ubuntu/run-pyxtest-valgrind.sh @@ -0,0 +1,78 @@ +#!/bin/bash +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 Enrico Weigelt, metux IT consult +# +# Build a debug Xvfb and run the pyxtest suite under valgrind memcheck. +# +# This is a dedicated, lightweight job: unlike run-xserver-build-and-test.sh +# it does not build Xorg/Xephyr/Xnest or fetch XTS/piglit/goxproto — pyxtest +# only exercises Xvfb here, and valgrind is slow enough already that we don't +# want to pay for the rest of the matrix too. It does still need a from-source +# xorgproto (util.sh's build_meson), since the Ubuntu runner image's +# presentproto is too old for meson.build's '>= 1.4' requirement. + +set -e + +if [ ! "$MESON_BUILDDIR" ]; then + echo "missing MESON_BUILDDIR" >&2 + exit 1 +fi + +if [ ! "$X11_BUILD_DIR" ]; then + echo "missing X11_BUILD_DIR" >&2 + exit 1 +fi + +. .github/scripts/util.sh + +echo "=== MESON_BUILDDIR=$MESON_BUILDDIR" +echo "=== X11_PREFIX=$X11_PREFIX" +echo "=== PKG_CONFIG_PATH=$PKG_CONFIG_PATH" + +mkdir -p "$X11_BUILD_DIR" +( + cd "$X11_BUILD_DIR" + build_meson xorgproto $(fdo_mirror xorgproto) "$PKG_XORGPROTO_REF" +) + +meson setup "$MESON_BUILDDIR" $MESON_ARGS +meson compile -C "$MESON_BUILDDIR" + +export XSERVER_BUILDDIR="$PWD/$MESON_BUILDDIR" +export XVFB_PATH="$XSERVER_BUILDDIR/hw/vfb/Xvfb" +export PYTHONDONTWRITEBYTECODE=1 + +# Per-test valgrind XML (+ a .txt summary when a test has findings) goes +# here, independent of pass/fail — see test/pyxtest/conftest.py's +# --valgrind-log-dir. Without this, valgrind's own output never leaves an +# ephemeral temp file, so a green run leaves no trace of what it checked +# and a failing run's report isn't visible without an API round-trip. +export VALGRIND_LOG_DIR="$PWD/valgrind-logs" +mkdir -p "$VALGRIND_LOG_DIR" + +# Run without -e so a failing/valgrind-tripped test still lets us write the +# job summary below before propagating the real exit code. +set +e +pytest -v --tb=short -n auto --timeout=300 --valgrind test/pyxtest/ +PYTEST_STATUS=$? +set -e + +if [ "${GITHUB_STEP_SUMMARY:-}" ]; then + { + echo "### Valgrind memcheck summary" + echo + shopt -s nullglob + findings=("$VALGRIND_LOG_DIR"/*.txt) + if [ "${#findings[@]}" -eq 0 ]; then + echo "No valgrind findings. Full per-test XML reports (leak-only included) are in the \`valgrind-logs\` artifact." + else + echo "${#findings[@]} test/server combination(s) with valgrind findings (full detail in the \`valgrind-logs\` artifact):" + echo + for f in "${findings[@]}"; do + echo "- \`$(basename "${f%.txt}")\`" + done + fi + } >>"$GITHUB_STEP_SUMMARY" +fi + +exit "$PYTEST_STATUS" diff --git a/.github/workflows/build-xserver.yml b/.github/workflows/build-xserver.yml index c7ec1969d6..a70abe8704 100644 --- a/.github/workflows/build-xserver.yml +++ b/.github/workflows/build-xserver.yml @@ -249,6 +249,57 @@ jobs: __BUILD/meson-logs/* __BUILD/test/piglit-results/* + xserver-build-ubuntu-valgrind: + # Dedicated lane for the pyxtest suite under valgrind memcheck + # (out-of-bounds reads/writes, use-after-free, uninitialised-value + # use). Kept separate from xserver-build-ubuntu* because valgrind is + # slow and this only needs Xvfb, not the full DDX/XTS/piglit matrix. + env: + MESON_ARGS: --buildtype=debug -Dprefix=/usr -Dxorg=false -Dxvfb=true -Dxnest=false -Dxephyr=false -Dwerror=true + runs-on: ubuntu-latest + needs: ubuntu-fetch-pkg + steps: + - name: Check out repository code + uses: actions/checkout@v6 + + - uses: ./.github/actions/ubuntu-install-pkg + + - name: prepare build environment + run: | + MACHINE=`gcc -dumpmachine` + echo "MACHINE=$MACHINE" >> "$GITHUB_ENV" + echo "PKG_CONFIG_PATH=$X11_PREFIX/share/pkgconfig:$X11_PREFIX/lib/$MACHINE/pkgconfig:$X11_PREFIX/lib/pkgconfig:$PKG_CONFIG_PATH" >> "$GITHUB_ENV" + + - name: X11 prereq cache + uses: actions/cache@v5 + with: + path: | + ${{ env.X11_PREFIX }} + key: ${{ runner.name }}-x11-deps-xorgproto-${{ hashFiles('.github/scripts/ubuntu/run-pyxtest-valgrind.sh') }} + restore-keys: ${{ runner.name }}-x11-deps-xorgproto- + + - name: build and run pyxtest under valgrind + run: .github/scripts/ubuntu/run-pyxtest-valgrind.sh + + - name: archive build logs + uses: actions/upload-artifact@v4 + with: + name: build-logs-valgrind + path: | + __BUILD/meson-logs/* + + - name: archive valgrind reports + # Upload even when the pytest step above failed - a failing + # run is exactly when the report is most needed, and it's + # what triggered writing this step's `if` in the first place. + if: always() + uses: actions/upload-artifact@v4 + with: + name: valgrind-logs + path: | + valgrind-logs/* + if-no-files-found: ignore + ubuntu-deps-image: # Content-addressed base image (apt deps + from-source prereqs baked in), # consumed by build-sdk-image below. The tag is a hash of the image's diff --git a/test/pyxtest/README.md b/test/pyxtest/README.md index a32f843ad3..ae63f697ad 100644 --- a/test/pyxtest/README.md +++ b/test/pyxtest/README.md @@ -107,6 +107,7 @@ pytest test/pyxtest/ -v --server-type=xvfb --server-type=xwayland |--------------------------------|-------------------------------------------| | `--valgrind` | Run all X servers under valgrind memcheck | | `--valgrind-suppressions=PATH` | Path to a valgrind suppressions file | +| `--valgrind-log-dir=PATH` | Persist a per-test valgrind XML report here (plus a `.txt` summary when there are findings), independent of pass/fail. Also settable via `VALGRIND_LOG_DIR`. | | `--server-type=TYPE` | Server type to test (`xvfb`, `xwayland`, `xorg`). Repeatable. Default: `xvfb` | | `--server-path=PATH` | Explicit path to the X server binary | | `--display=DISPLAY` | Connect to an existing server instead of starting one. Accepts `:N` or `N`. Mutually exclusive with `--valgrind` and `--server-path` | @@ -123,6 +124,8 @@ The server binary is located by checking, in order: `VALGRIND_SUPPRESSIONS` can point to a suppressions file. +`VALGRIND_LOG_DIR` is the environment-variable form of `--valgrind-log-dir`. + `XSERVER_ASAN` set to `1` indicates the server binary was built with AddressSanitizer. This is set automatically by meson when `-Db_sanitize=address` is used. It can also be set manually. diff --git a/test/pyxtest/conftest.py b/test/pyxtest/conftest.py index cf8bf141d5..1368076684 100644 --- a/test/pyxtest/conftest.py +++ b/test/pyxtest/conftest.py @@ -3,7 +3,9 @@ # pytest configuration and fixtures for X server testing. import os +import re import shutil +import warnings import pytest from pathlib import Path @@ -23,6 +25,16 @@ def pytest_addoption(parser): default=None, help="Path to valgrind suppressions file", ) + parser.addoption( + "--valgrind-log-dir", + default=None, + help="Directory to persist a per-test valgrind XML report into " + "(also honours the VALGRIND_LOG_DIR environment variable). " + "Written for every test run under valgrind, independent of " + "pass/fail, since the XML otherwise only exists in an ephemeral " + "temp file. A short human-readable .txt summary is written " + "alongside whenever valgrind reported anything.", + ) parser.addoption( "--server-type", action="append", @@ -61,6 +73,10 @@ def pytest_configure(config): "markers", "swapped_client: mark test as requiring a byte-swapped client" ) + log_dir = get_valgrind_log_dir(config) + if log_dir is not None: + log_dir.mkdir(parents=True, exist_ok=True) + # Validate --display against conflicting options display = config.getoption("--display", default=None) if display is not None: @@ -113,6 +129,24 @@ def get_valgrind_suppressions(config) -> Path | None: return None +def get_valgrind_log_dir(config) -> Path | None: + """Find the directory to persist per-test valgrind reports into.""" + explicit = config.getoption("--valgrind-log-dir") + if explicit: + return Path(explicit) + + env = os.environ.get("VALGRIND_LOG_DIR") + if env: + return Path(env) + + return None + + +def _sanitize_test_id(nodeid: str) -> str: + """Turn a pytest nodeid into a safe filename fragment.""" + return re.sub(r"[^A-Za-z0-9._-]+", "_", nodeid) + + def is_valgrind_available() -> bool: """Check if valgrind is available on the system.""" return shutil.which("valgrind") is not None @@ -163,6 +197,36 @@ def pytest_generate_tests(metafunc): metafunc.parametrize("xserver", server_types, indirect=True) +def _persist_valgrind_report(request, server_type, xml_path, errors): + """Copy a test's valgrind XML into --valgrind-log-dir, if configured. + + Runs for every test executed under valgrind, independent of whether + the test passed, failed, or only found leaks (which don't fail the + test) - otherwise the XML never leaves the ephemeral temp file it + was written to. Writes a short .txt summary alongside whenever + valgrind reported anything, so CI can surface findings without + parsing XML. + """ + log_dir = get_valgrind_log_dir(request.config) + if log_dir is None: + return + + stem = f"{_sanitize_test_id(request.node.nodeid)}-{server_type}" + dest = log_dir / f"{stem}.xml" + try: + shutil.copyfile(xml_path, dest) + except OSError as e: + warnings.warn(f"Failed to persist valgrind XML for {stem}: {e}", stacklevel=2) + return + + if errors: + summary = log_dir / f"{stem}.txt" + summary.write_text( + f"{len(errors)} valgrind finding(s) for {request.node.nodeid} " + f"({server_type}):\n\n" + "\n\n".join(str(e) for e in errors) + "\n" + ) + + def _start_server(request, server_type, log_file=None): """Start an X server of the given type for a test. @@ -212,6 +276,11 @@ def _start_server(request, server_type, log_file=None): valgrind_errors = server.stop() + if use_valgrind and server.valgrind_xml_path is not None: + _persist_valgrind_report( + request, server_type, server.valgrind_xml_path, valgrind_errors + ) + if use_asan and asan_errors: msg = f"AddressSanitizer found {len(asan_errors)} error(s):\n\n" msg += "\n\n".join(str(e) for e in asan_errors) diff --git a/test/pyxtest/xserver.py b/test/pyxtest/xserver.py index f106d6f35e..1a5e6ac0e7 100644 --- a/test/pyxtest/xserver.py +++ b/test/pyxtest/xserver.py @@ -451,6 +451,17 @@ class XServerProcess: pass return "" + @property + def valgrind_xml_path(self) -> Path | None: + """Path to this server's valgrind XML output, if valgrind was used. + + Stays valid after :meth:`stop`, so callers can persist it + elsewhere (the file itself is not deleted automatically). + """ + if self._valgrind_xml_file is None: + return None + return Path(self._valgrind_xml_file.name) + def check_valgrind_errors(self): """Parse and assert no valgrind errors occurred.""" if self._valgrind_xml_file is None: