diff --git a/mtproxymax.sh b/mtproxymax.sh index b0ac44b..a2cbd86 100644 --- a/mtproxymax.sh +++ b/mtproxymax.sh @@ -1777,6 +1777,38 @@ _load_all_cumulative_user_stats() { done } +# util-linux flock takes `-w SECS` to wait for a lock; busybox's flock applet does not, +# and `command -v flock` cannot tell the two apart because busybox installs a `flock` +# symlink to /bin/busybox. Testing for the binary therefore succeeds on Alpine while the +# lock call itself fails. Probe the capability once instead of assuming it. +_FLOCK_WAIT_SUPPORTED="" +_flock_supports_wait() { + if [ -z "$_FLOCK_WAIT_SUPPORTED" ]; then + if command -v flock >/dev/null 2>&1 && + (exec 9>/dev/null; flock -w 0 9) 2>/dev/null; then + _FLOCK_WAIT_SUPPORTED="yes" + else + _FLOCK_WAIT_SUPPORTED="no" + fi + fi + [ "$_FLOCK_WAIT_SUPPORTED" = "yes" ] +} + +# Take an exclusive lock on the already-open file descriptor $1. +# Returns 0 when the lock is held, non-zero when it could not be taken. A host with no +# flock at all proceeds unlocked, which is the long-standing behaviour in that case. +_lock_fd() { + local _fd="${1:?file descriptor required}" + command -v flock >/dev/null 2>&1 || return 0 + if _flock_supports_wait; then + flock -w 5 "$_fd" 2>/dev/null + else + # busybox: -w is unavailable, so take the lock without waiting rather than not + # locking at all. Losing the 5s grace period is far better than skipping the lock. + flock -n "$_fd" 2>/dev/null + fi +} + # One-shot flush of traffic counters to disk (for use before stop/restart) # Works standalone — loads cumulative from disk, computes delta from live metrics, saves back flush_traffic_to_disk() { @@ -1787,7 +1819,14 @@ flush_traffic_to_disk() { mkdir -p "$_stats_dir" 2>/dev/null # Acquire lock to prevent race with daemon's save_traffic exec 9>"${_stats_dir}/.traffic.lock" - flock -w 5 9 2>/dev/null || { exec 9>&- 2>/dev/null; return 0; } + # Report failure rather than returning 0: this used to claim success while writing + # nothing, which silently discarded counters. Plain stderr, not log_error(), because + # this also runs from the daemon, which has no logging helpers of its own. + if ! _lock_fd 9; then + echo "mtproxymax: could not acquire traffic lock — counters not flushed" >&2 + exec 9>&- + return 1 + fi # Load existing cumulative totals local cum_in=0 cum_out=0 @@ -2608,12 +2647,10 @@ secret_reset_traffic() { # disk update with save_traffic() and leave it a reset command to consume # before its next write, otherwise it would restore the pre-reset values. exec 9>"${STATS_DIR}/.traffic.lock" - if command -v flock &>/dev/null; then - flock -w 5 9 2>/dev/null || { - log_error "Could not acquire traffic lock. Telegram daemon may be busy." - exec 9>&- - return 1 - } + if ! _lock_fd 9; then + log_error "Could not acquire traffic lock. Telegram daemon may be busy." + exec 9>&- + return 1 fi if [ "$label" = "all" ]; then @@ -6627,8 +6664,10 @@ run_traffic_reset_global() { log_info "Resetting cumulative counters..." exec 9>"${_stats_dir}/.traffic.lock" - if command -v flock &>/dev/null; then - flock -w 5 9 2>/dev/null || { log_error "Could not acquire traffic lock. Daemon may be busy."; exec 9>&-; return 1; } + if ! _lock_fd 9; then + log_error "Could not acquire traffic lock. Daemon may be busy." + exec 9>&- + return 1 fi local _tmp @@ -11545,12 +11584,52 @@ load_traffic() { fi } +# Duplicated from the manager script: this heredoc has a quoted delimiter, so it is +# written out literally and the daemon never sources the manager. Without these two +# definitions save_traffic() below calls an undefined _lock_fd, gets 127 back, and +# returns before writing any counters — which is exactly the accounting loss this +# change set out to fix. Keep identical to _flock_supports_wait/_lock_fd above. +_FLOCK_WAIT_SUPPORTED="" +_flock_supports_wait() { + if [ -z "$_FLOCK_WAIT_SUPPORTED" ]; then + if command -v flock >/dev/null 2>&1 && + (exec 9>/dev/null; flock -w 0 9) 2>/dev/null; then + _FLOCK_WAIT_SUPPORTED="yes" + else + _FLOCK_WAIT_SUPPORTED="no" + fi + fi + [ "$_FLOCK_WAIT_SUPPORTED" = "yes" ] +} + +# Take an exclusive lock on the already-open file descriptor $1. +# Returns 0 when the lock is held, non-zero when it could not be taken. A host with no +# flock at all proceeds unlocked, which is the long-standing behaviour in that case. +_lock_fd() { + local _fd="${1:?file descriptor required}" + command -v flock >/dev/null 2>&1 || return 0 + if _flock_supports_wait; then + flock -w 5 "$_fd" 2>/dev/null + else + # busybox: -w is unavailable, so take the lock without waiting rather than not + # locking at all. Losing the 5s grace period is far better than skipping the lock. + flock -n "$_fd" 2>/dev/null + fi +} + save_traffic() { local _tdir="${INSTALL_DIR}/relay_stats" mkdir -p "$_tdir" 2>/dev/null # Acquire lock to prevent race with flush_traffic_to_disk exec 9>"${_tdir}/.traffic.lock" - flock -w 5 9 2>/dev/null || { exec 9>&- 2>/dev/null; return 0; } + # Fail loudly rather than returning 0: this used to report success while writing + # nothing, so on a host whose flock lacked -w the daemon never persisted counters and + # no error was ever surfaced. stderr rather than log_error() — see _lock_fd callers. + if ! _lock_fd 9; then + echo "mtproxymax: could not acquire traffic lock — traffic not saved" >&2 + exec 9>&- + return 1 + fi apply_pending_traffic_resets local _tmp=$(mktemp "${_tdir}/.traffic.XXXXXX" 2>/dev/null) || { exec 9>&-; return; } chmod 600 "$_tmp" @@ -12162,7 +12241,9 @@ while true; do _now=$(date +%s) if [ $((_now - _last_traffic_update)) -ge 60 ] && is_running; then _last_traffic_update=$_now - update_traffic 2>/dev/null + # Deliberately not silenced: save_traffic() reports a failed lock on stderr, and + # discarding that is what let unpersisted counters go unnoticed in production. + update_traffic [ "${PORTAL_ENABLED:-false}" = "true" ] && "${INSTALL_DIR}/mtproxymax" portal generate &>/dev/null # Connection log: append per-user activity (delta = current cumulative - previous cumulative) @@ -12473,12 +12554,18 @@ save_replication() { chmod 600 "$tmp" # Serialise with sync-timer flock to prevent lost-update races with save_sync_status() - exec 201>"${INSTALL_DIR:-/opt/mtproxymax}/.mtproxymax-sync.lock" 2>/dev/null || true - if command -v flock &>/dev/null; then - flock -w 5 201 2>/dev/null || { log_error "Could not acquire lock for replication config"; rm -f "$tmp"; exec 201>&- 2>/dev/null; return 1; } + # No 2>/dev/null here: `exec` with only redirections applies to the whole shell, so it + # would silence stderr for the rest of the process — including the log_error below, + # which is exactly the failure this function is supposed to report. + exec 201>"${INSTALL_DIR:-/opt/mtproxymax}/.mtproxymax-sync.lock" || true + if ! _lock_fd 201; then + log_error "Could not acquire lock for replication config" + rm -f "$tmp" + exec 201>&- + return 1 fi mv "$tmp" "$REPLICATION_FILE" - exec 201>&- 2>/dev/null || true + exec 201>&- || true } # Load replication.conf @@ -12800,7 +12887,7 @@ do_sync() { main() { # Prevent overlapping sync runs - exec 200>"${LOCK_FILE}" 2>/dev/null || true + exec 200>"${LOCK_FILE}" || true if command -v flock &>/dev/null; then flock -n 200 || { echo "[$(date '+%Y-%m-%d %H:%M:%S')] SKIP: Another sync already running" >> "${REPLICATION_LOG}" diff --git a/tests/test_flock_portability.sh b/tests/test_flock_portability.sh new file mode 100644 index 0000000..aebaf58 --- /dev/null +++ b/tests/test_flock_portability.sh @@ -0,0 +1,179 @@ +#!/bin/bash +# Regression tests for lock acquisition on hosts whose flock is busybox's applet. +# +# busybox ships a `flock` that has no -w option. Because it is present on PATH, +# `command -v flock` succeeds and the "flock is unavailable, skip locking" guard never +# fires — but `flock -w 5 9` itself fails. On Alpine that made every traffic reset abort +# with a misleading "Could not acquire traffic lock" message, and made two save paths +# report success while writing nothing at all. +# +# The busybox behaviour is reproduced with a stub on PATH rather than by requiring an +# Alpine host, so this regression test runs on every platform in the CI matrix. +set -o pipefail + +if [ "${BASH_VERSINFO[0]:-0}" -lt 4 ]; then + echo "SKIP: bash 4+ required (got ${BASH_VERSION:-unknown})" >&2 + exit 0 +fi + +TEST_TMPDIR=$(mktemp -d 2>/dev/null || mktemp -d -t 'mtp_flock_XXXXXX') +INSTALL_DIR="$TEST_TMPDIR/install" +mkdir -p "$INSTALL_DIR/relay_stats" + +# A flock that behaves like busybox's: it accepts -n/-s/-x/-u but rejects -w outright. +# FAKE_FLOCK_HOLD=1 additionally makes every acquire fail, to exercise the other branch. +FAKEBIN="$TEST_TMPDIR/bin" +mkdir -p "$FAKEBIN" +cat >"$FAKEBIN/flock" <<'FLOCK_EOF' +#!/bin/bash +for _a in "$@"; do + case "$_a" in + -w*) echo "flock: unrecognized option: w" >&2; exit 1 ;; + esac +done +[ "${FAKE_FLOCK_HOLD:-0}" = "1" ] && exit 1 +exit 0 +FLOCK_EOF +chmod +x "$FAKEBIN/flock" +PATH="$FAKEBIN:$PATH" +export PATH + +MTPROXYMAX_SOURCE_ONLY=true source "$(dirname "${BASH_SOURCE[0]}")/../mtproxymax.sh" +set +e +trap 'rm -rf "$TEST_TMPDIR"' EXIT + +TESTS_RUN=0 +TESTS_FAILED=0 +LAST_ERROR="" + +assert_eq() { + local name="$1" want="$2" got="$3" + TESTS_RUN=$((TESTS_RUN + 1)) + if [ "$got" = "$want" ]; then + printf ' PASS %s\n' "$name" + else + printf ' FAIL %s (got=%q want=%q)\n' "$name" "$got" "$want" + TESTS_FAILED=$((TESTS_FAILED + 1)) + fi +} + +check_root() { :; } +load_settings() { :; } +reload_proxy_config() { :; } +log_info() { :; } +log_success() { :; } +log_error() { LAST_ERROR="$*"; } +audit_log() { :; } + +SECRETS_LABELS=(alice) +SECRETS_ENABLED=(true) + +METRICS='# HELP test test +telemt_user_octets_from_client{user="alice"} 120 +telemt_user_octets_to_client{user="alice"} 340' + +_fetch_metrics() { printf '%s\n' "$METRICS"; } +curl() { printf '%s\n' "$METRICS"; } + +echo "flock portability tests" + +# The stub must really be the flock that resolves, or the test proves nothing. +assert_eq "busybox-style flock is the one on PATH" "$FAKEBIN/flock" "$(command -v flock)" + +# --- a busybox flock must not prevent the lock from being taken ----------------- +printf 'alice|1000|2000\n' >"$STATS_DIR/user_traffic" +printf 'alice|10|20\n' >"$STATS_DIR/user_traffic_snapshot" + +secret_reset_traffic alice no_reload +assert_eq "traffic reset succeeds despite flock lacking -w" "0" "$?" +assert_eq "reset work actually happened" "present" \ + "$([ -f "$STATS_DIR/.traffic_reset_pending" ] && echo present || echo absent)" + +# --- an unavailable lock must fail loudly, never silently succeed --------------- +# Both save paths used to `return 0` here, reporting success while writing nothing. +FAKE_FLOCK_HOLD=1 +export FAKE_FLOCK_HOLD +printf 'alice|1000|2000\n' >"$STATS_DIR/user_traffic" +rm -f "$STATS_DIR/.traffic_reset_pending" + +secret_reset_traffic alice no_reload +assert_eq "held lock makes the reset fail" "1" "$?" +assert_eq "held lock does not silently succeed" "absent" \ + "$([ -f "$STATS_DIR/.traffic_reset_pending" ] && echo present || echo absent)" + +flush_traffic_to_disk +assert_eq "flush_traffic_to_disk fails when the lock is unavailable" "1" "$?" + +unset FAKE_FLOCK_HOLD + +# --- the generated daemon must carry its own copy of the lock helpers ------------ +# The bot daemon is emitted from a single-quoted heredoc and never sources the +# manager, so every helper it calls has to be defined inside that heredoc. These two +# helpers were added only to the manager, which left the generated daemon calling an +# undefined _lock_fd: save_traffic() then returned 1 and persisted no counters at all. +# Everything above passes either way, because it exercises the manager, not the daemon. +TEST_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +DAEMON_FULL="$TEST_TMPDIR/daemon_full.sh" +awk "/<<[[:space:]]*'TELEGRAM_SCRIPT'/{f=1;next} /^TELEGRAM_SCRIPT\$/{f=0} f" \ + "$TEST_ROOT/mtproxymax.sh" >"$DAEMON_FULL" +assert_eq "generated daemon captured" "yes" \ + "$([ -s "$DAEMON_FULL" ] && echo yes || echo no)" + +# Declarations and function bodies only: stop before the cleanup trap so that +# sourcing this cannot start the daemon's main loop. +DAEMON_PRELUDE="$TEST_TMPDIR/daemon_prelude.sh" +awk '/^trap /{exit} {print}' "$DAEMON_FULL" >"$DAEMON_PRELUDE" + +assert_eq "daemon defines _lock_fd" "yes" \ + "$(grep -qE '^_lock_fd\(\)' "$DAEMON_FULL" && echo yes || echo no)" +assert_eq "daemon defines _flock_supports_wait" "yes" \ + "$(grep -qE '^_flock_supports_wait\(\)' "$DAEMON_FULL" && echo yes || echo no)" + +# Exercise the daemon's own save_traffic(), not the manager's. +DAEMON_HARNESS="$TEST_TMPDIR/daemon_save_traffic.sh" +cat >"$DAEMON_HARNESS" <<'HARNESS_EOF' +#!/bin/bash +source "$2" +INSTALL_DIR="$1" +TRAFFIC_FILE="${INSTALL_DIR}/relay_stats/cumulative_traffic" +USER_TRAFFIC_FILE="${INSTALL_DIR}/relay_stats/user_traffic" +mkdir -p "${INSTALL_DIR}/relay_stats" +_cum_in=700; _cum_out=900 +_cum_user_in=([alice]=11); _cum_user_out=([alice]=22) +_prev_user_in=([alice]=1); _prev_user_out=([alice]=2) +_prev_total_in=5; _prev_total_out=6 +save_traffic +echo "rc=$?" +HARNESS_EOF + +DAEMON_INSTALL="$TEST_TMPDIR/daemon_install" +daemon_run() { + rm -rf "$DAEMON_INSTALL" + bash "$DAEMON_HARNESS" "$DAEMON_INSTALL" "$DAEMON_PRELUDE" 2>&1 +} +daemon_file() { cat "$DAEMON_INSTALL/relay_stats/$1" 2>/dev/null; } + +# The daemon is the only writer of accounting in production, and this is the path +# that silently stopped persisting on every distro, not just Alpine. +_dout=$(daemon_run) +assert_eq "daemon save_traffic succeeds under busybox flock" "rc=0" \ + "$(printf '%s\n' "$_dout" | grep '^rc=')" +assert_eq "daemon wrote cumulative_traffic" "700|900" "$(daemon_file cumulative_traffic)" +assert_eq "daemon wrote user_traffic" "alice|11|22" "$(daemon_file user_traffic)" +assert_eq "daemon wrote user_traffic_snapshot" "alice|1|2" \ + "$(daemon_file user_traffic_snapshot)" +assert_eq "daemon wrote global_traffic_snapshot" "5|6" \ + "$(daemon_file global_traffic_snapshot)" + +# A lock that is genuinely held must fail loudly rather than persist nothing. +FAKE_FLOCK_HOLD=1 +export FAKE_FLOCK_HOLD +_dout=$(daemon_run) +assert_eq "daemon save_traffic fails when the lock is held" "rc=1" \ + "$(printf '%s\n' "$_dout" | grep '^rc=')" +assert_eq "daemon reports the failure on stderr" "yes" \ + "$(printf '%s\n' "$_dout" | grep -q 'traffic not saved' && echo yes || echo no)" +unset FAKE_FLOCK_HOLD + +printf '\n%d tests, %d failures\n' "$TESTS_RUN" "$TESTS_FAILED" +[ "$TESTS_FAILED" -eq 0 ]