build: autotools-to-CMake migration#523
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a CMake-based build path and CI coverage to validate CMake builds on Linux and start surfacing Windows porting errors early, without touching the existing C sources.
Changes:
- Introduces a top-level
CMakeLists.txtthat buildsspineand generatesconfig/config.hviaconfigure_file(). - Adds a CMake config header template (
config/config.h.cmake.in) to mirror key autotools feature macros. - Extends GitHub Actions CI with Linux CMake builds and a Windows MSYS2/MinGW build job (plus artifact upload/crash-dump plumbing).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
config/config.h.cmake.in |
New CMake-driven config.h template for feature macros and build-time constants. |
CMakeLists.txt |
New CMake build definition for dependency detection, config generation, build, and install. |
.github/workflows/ci.yml |
Adds Linux CMake job and Windows MSYS2/MinGW job; minor whitespace cleanup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add CMakeLists.txt mirroring configure.ac feature checks - Add config/config.h.cmake.in template for cmake builds - Add build-cmake-linux CI job (gcc/clang matrix) - Add build-windows CI job (MSYS2/MinGW-w64, continue-on-error) - Windows crash dump collection via WER LocalDumps - Gate -Wall by compiler ID (GNU/Clang vs MSVC) - Use CMAKE_DL_LIBS instead of hardcoded -ldl - Parse net-snmp-config --libs into proper NETSNMP_LIBRARIES - Add SNMP_LOCALNAME compile check for feature parity - No C source files modified Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
d36fb77 to
7c94f7e
Compare
The PHP Script Server spawn passed raw environ to posix_spawn, so any LD_PRELOAD, DYLD_INSERT_LIBRARIES, BASH_ENV, or similar injection in spine's parent environment would propagate into every PHP worker. Expose spine_build_child_env (already used by nft_popen) and apply it here so the two spawn paths share one filter. Fall back to raw environ on allocation failure to keep the poller running. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
The ICMP on-wire seq field is 16 bits, so a wider unsigned int counter wraps incorrectly from the hardware's perspective before it wraps in memory. Use _Atomic uint16_t under C11 so the type matches the wire format and atomic_fetch_add_explicit gives us lock-free relaxed increments across poller threads. Keep the __atomic builtin fallback for toolchains without <stdatomic.h>. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
The option reads a 1-byte boolean through the passed pointer. Passing a plain C99 bool works on MySQL 8+ where my_bool was removed but risks reading extra bytes on MariaDB or MySQL <8.0, where my_bool is a char typedef. Gate the type on MARIADB_BASE_VERSION / MYSQL_VERSION_ID so each connector gets the width it expects. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Introduce a small C façade so ping logic does not have to fan out into OS-specific code paths. The POSIX side is backed by ping.c; the Windows side will load iphlpapi dynamically in a follow-up. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Windows uses LoadLibraryW("iphlpapi.dll") + GetProcAddress so spine
starts on stripped SKUs where iphlpapi is absent, with the failure
surfacing as a clean SPINE_ICMP_ERROR instead of a load-time crash.
Status codes map to the enum in platform_icmp.h. POSIX forwards into
ping.c's raw-socket helpers so the existing authoritative path stays
in one place.
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Extract the payload signature check and the link-local scope_id resolver into tiny standalone TUs so unit tests can link them without dragging in the full spine dependency chain (mysql, net-snmp, the poller). Behavior is byte-identical to the previous inline definitions in ping.c. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
IPv6 socket setup now requests ICMP6_FILTER (only echo replies), IPV6_CHECKSUM (offset for the kernel-computed csum), and IPV6_UNICAST_HOPS. Link-local destinations get a sin6_scope_id resolved from the first non-loopback v6 interface when the caller did not supply a %zone suffix. Each echo now carries a SPINE_PING_MAGIC + pid_mask signature so a reply that happens to match id+seq but originates from an unrelated flow is still dropped. Receive paths bounds-check before the struct cast, reject undersized packets, and verify source, id, seq, and payload signature in that order. getaddrinfo in resolve_sockaddr now sets AI_NUMERICHOST when the hostname is a numeric literal so a crafted dotted-quad lookalike cannot steer the resolver into DNS. CAP_NET_RAW drop is wired behind HAVE_LIBCAP but deferred at the call site because spine opens raw sockets on demand per ping; a future persistent-socket refactor can enable the call unchanged. Numeric-address oneshot helpers at the bottom of ping.c back the platform_icmp facade without duplicating the capability dance. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Network-free tests covering the happy path plus the rejection cases that matter: wrong magic, wrong pid_mask, undersized buffer, NULL input. The scope_id test tolerates minimal CI containers that lack a usable non-loopback IPv6 interface. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Three HIGH + one MEDIUM from the review hook: 1. Oneshot helpers recompute remaining timeout each loop, so a flood of mismatched replies cannot extend the deadline. 2. Windows loader publishes function pointers with MemoryBarrier() before InterlockedExchange(&g_load_ok, 1), fixing the ARM64 race where a waiter could observe g_load_ok=1 with NULL fn pointers. Waiter now gates on g_load_ok alone. 3. Windows facade handles payload==NULL by building a default signature (matches POSIX behaviour) and rejects the degenerate NULL+len>0 case before forwarding to iphlpapi.dll. 4. Bare free() in cleanup paths replaced with SPINE_FREE() per project convention. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Address the second round of review findings: - New src/ping_wire.h defines SPINE_PING_MAGIC and spine_ping_payload_t once, with a C11 _Static_assert on the 16-byte layout. The three TUs that touched the on-wire format (ping.c, ping_validate.c, platform_icmp_win.c) now all include this header so a future edit cannot silently drift. - Numeric-address oneshot helpers now call spine_ping_validate_payload() on replies when the facade owns payload composition (payload argument was NULL). Closes the gap where spine_icmp_echo_v4/v6 were weaker than the host_t-driven path at rejecting LAN spoofs. - Replace remaining bare free() with SPINE_FREE() in ping.c and a local equivalent (SPINE_ICMP_FREE) in the Windows facade TU. - Reply-validation test uses the shared wire header and checks sizeof(spine_ping_payload_t) == 16 at runtime. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
|
Pushed
Pre-push review passed (Stage 1 CLEAN, Stage 2 CLEAN, Verdict PASS). |
Adds Linux systemd integration that compiles into spine via libsystemd when present (pkg-config detected, WITH_SYSTEMD=ON by default). On macOS, Windows, and minimal Linux without libsystemd, sd_notify calls become no-op stubs and the build is unchanged. Runtime: - READY=1 published once main initialization completes. - WATCHDOG=1 emitted from the poller cycle so systemd can restart a hung instance via WatchdogSec=120 in the unit. - STOPPING=1 with status text on graceful shutdown. - STATUS= updated periodically with cycle and host counts. - SIGHUP triggers RELOADING=1, re-reads spine.conf, then re-publishes READY=1 with a fresh MONOTONIC_USEC. Unit (etc/systemd/spine.service): - Type=notify with NotifyAccess=main, Restart=on-failure. - Drops to dedicated spine user/group; AmbientCapabilities=CAP_NET_RAW is the only privilege. - Hardening: NoNewPrivileges, ProtectSystem=strict, ProtectHome, ProtectKernel*, ProtectClock, ProtectHostname, RestrictNamespaces, RestrictRealtime, RestrictSUIDSGID, LockPersonality, MemoryDenyWriteExecute, SystemCallArchitectures=native, SystemCallFilter=@System-service excluding @PRIVILEGED @resources, RestrictAddressFamilies=AF_INET AF_INET6 AF_UNIX AF_NETLINK. - ReadWritePaths scoped to /var/log/cacti and /var/run/spine only. - Journal logging via StandardOutput=journal. Companion etc/systemd/spine.timer drives 5-minute polls when spine is deployed without an external scheduler. Install paths use systemd's pkg-config systemdsystemunitdir variable with /lib/systemd/system as the LTS-distro fallback. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
|
Added idiomatic systemd integration (4ca79f2). Type=notify with sd_notify wiring for READY/WATCHDOG/STOPPING/STATUS plus SIGHUP reload (RELOADING + READY republish). The unit hardens with NoNewPrivileges, ProtectSystem=strict, MemoryDenyWriteExecute, SystemCallFilter=@System-service excluding @PRIVILEGED @resources, and AmbientCapabilities=CAP_NET_RAW as the only privilege. libsystemd is detected via pkg-config; macOS, Windows, and minimal Linux without it build unchanged via no-op stubs. Companion spine.timer included for deployments without an external scheduler. |
If clock_gettime(CLOCK_MONOTONIC) fails (vDSO issues, sandbox restrictions), sending MONOTONIC_USEC=0 makes systemd interpret the reload as starting before boot. Send RELOADING=1 without the timestamp so systemd falls back to the receive time. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Guards against regressions where the libsystemd-absent code path stops compiling on Linux. macOS and Windows exercise this implicitly, but an explicit Linux lane catches breakage on the primary CI platform. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Covers double-call safety (SIGTERM + main exit can both call spine_sd_stopping), NULL status strings, and the libsystemd-absent no-op path. Test passes under both WITH_SYSTEMD=ON and OFF. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
spine_sd_status(NULL) now short-circuits to avoid vsnprintf(NULL) UB. The fprintf fallback in spine_sd_reloading keeps the TU decoupled from spine.h so it is safe in signal handlers and pre-init contexts; Type=notify captures stderr into the journal. Test exercises the NULL contract. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Correct the comment in spine_sd_reloading: the reload path runs from normal SIGHUP handling (via signalfd/self-pipe dispatch in the main loop), not from a raw signal handler. stdio buffering is safe there. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Nine workflows still invoked ./configure after the autotools removal, causing every analysis/coverage/fuzz/integration job to fail with './configure: No such file or directory'. Each now uses cmake -B build + cmake --build build with the same compiler/flags injected via -DCMAKE_C_COMPILER and -DCMAKE_C_FLAGS. Updated workflows: codeql, coverage, fuzzing, integration, nightly, perf-regression, release-verification, static-analysis, weekly. Coverage uses lcov + ctest. scan-build wraps the cmake configure and build invocations. cppcheck include paths updated for src/ layout. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
TheWitness
left a comment
There was a problem hiding this comment.
Readme should read something like:
- Prerequisites
a. EL or Variants
dnf install a b c d
b. Debian and Variants
apt-get install a b c
Not seeing that. Also the below. Not making on Rocky9.
[9/50] Building C object CMakeFiles/spine_platform.dir/src/platform/platform_process_posix.c.o
FAILED: CMakeFiles/spine_platform.dir/src/platform/platform_process_posix.c.o
/usr/bin/cc -D_DEFAULT_SOURCE=1 -D_POSIX_C_SOURCE=200809L -I/var/www/html/spine_new/build -I/var/www/html/spine_new -I/var/www/html/spine_new/src -I/var/www/html/spine_new/src/platform -I/var/www/html/spine_new/third_party -std=gnu17 -Wall -Wextra -Wformat -Werror=implicit-function-declaration -Werror=incompatible-pointer-types -Wvla -Wshadow -D_FORTIFY_SOURCE=2 -fstack-protector-strong -Wformat-security -fPIE -MD -MT CMakeFiles/spine_platform.dir/src/platform/platform_process_posix.c.o -MF CMakeFiles/spine_platform.dir/src/platform/platform_process_posix.c.o.d -o CMakeFiles/spine_platform.dir/src/platform/platform_process_posix.c.o -c /var/www/html/spine_new/src/platform/platform_process_posix.c
|
…on glibc platform_process_posix.c builds with _POSIX_C_SOURCE=200809L, which hides the Linux-only pipe2(2) prototype on glibc. The macro must be defined before any libc header pulls in <features.h>, which latches the exposed symbol set on first inclusion. Moving the define above the transitive <sys/types.h> include in platform_process.h restores the declaration and unblocks builds on Rocky Linux 9, AlmaLinux 9, and other glibc-based distros where -Werror=implicit-function-declaration was aborting the build. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
|
Built and tested spine across 10 Linux distros locally via Docker: PASS (10/10): rockylinux:9, rockylinux:8, almalinux:9, fedora:latest, debian:12, debian:trixie, ubuntu:22.04, ubuntu:24.04, opensuse/leap:15, alpine:3.20 Fixes applied:
Added Anyone can reproduce any row locally with |
bc3e446 to
f43b906
Compare
somethingwithproof
left a comment
There was a problem hiding this comment.
This PR represents a major modernization and hardening of the Spine poller, introducing robust platform abstractions, environment filtering for child processes, and enhanced ICMP security measures. The code quality is generally high, with a clear focus on security-first design and error handling.
🔍 General Feedback
- Excellent use of environment filtering (
spine_build_child_env) to prevent hijacking via dynamic linker or shell startup. - Strong ICMP security with random ID masking and payload signature validation.
- Modernization via
posix_spawnandO_CLOEXECsignificantly improves process isolation and prevents descriptor leakage. - Transition to CMake provides better dependency detection and build-time hardening.
Detailed Findings
-
HIGH: Command Injection in exec_poll (src/poller.c:2418)
- Spine executes script commands directly via the system shell. If the Cacti database is compromised, an attacker can gain RCE by modifying script entries.
- Recommendation: Ensure the Cacti web interface strictly validates script paths and arguments. Consider an optional "Strict Command Policy" for enhanced security.
-
MEDIUM: Potential Command Injection in php_cmd (src/php.c:66)
- Writing unescaped strings to the PHP script server pipe could allow for command injection if newlines or other special characters are injected into the script server protocol.
- Recommendation: Validate
php_commandfor newlines and protocol-subverting characters.
-
LOW: Potential SQL Injection in getsetting (src/util.c:101)
- The
settingparameter is used unescaped in SQL queries. While currently only used with literals, it represents a risk if reused with untrusted input in the future. - Recommendation: Use
db_escapeon thesettingargument.
- The
-
LOW: Potential snprintf truncation (src/util.c:783)
- The formatted JSON string could theoretically exceed
BUFSIZE(1024) ifspine_authandspine_privwere near their capacity. While hardcoded values are currently short, increasing the buffer size or guarding lengths would eliminate the compiler warning. - Recommendation: Increase the
spine_capabilitiesbuffer size or explicitly guard the lengths.
- The formatted JSON string could theoretically exceed
Scope
This PR is the consolidated
developtrack for Spine platform/build modernization and runtime hardening.It includes:
Newly Added In This Round
command_policy.c/.h)exec_poll()to reject unsafe shell metacharactersdocker compose run --no-depswhere appropriate.dockerignore+ image build behavior)Why Consolidation
Prior split PRs overlapped heavily in
poller.c,spine.c,util.c, platform layers, and CI files, which repeatedly caused merge-order and conflict risk.This single PR keeps the merge path deterministic and avoids duplicate conflict resolution across parallel PRs.
Superseded PRs
Closed as superseded by this PR: #512, #524, #525, #526, #527, #528, #529, #530, #531, #532.
Validation Performed
cmake --preset ci-smokecmake --build --preset ci-smokectest --test-dir build --output-on-failuretests/integration/test_script_command_policy.shtests/integration/test_ipv6_transport.sh.github/scripts/check-workflow-policy.pyAll above passed locally on this branch.