Fix timer.cancel() deadlock and add MAC format validation

Two P1 bugs fixed:

1. Timer cancel deadlock: timer.cancel() was called while holding locks, which would deadlock if the callback was running/paused waiting for those same locks. Fixed by extracting timers outside of all locks before calling cancel().

2. MAC validation: _normalize_mac() now validates format with regex and returns empty string for invalid MACs. Callers (load_personal_macs_from_env, is_personal_device) skip empty results and log warnings.

All 16 tests pass.
This commit is contained in:
Cobra
2026-04-12 08:41:04 -04:00
parent 56801c6baa
commit de7cfe552c
+55 -29
View File
@@ -13,6 +13,7 @@ Three concurrent threads:
import json import json
import logging import logging
import os import os
import re
import socket import socket
import struct import struct
import sys import sys
@@ -365,9 +366,15 @@ def setup_logging():
return logger return logger
# MAC address validation regex: exactly 12 hex digits (after normalization)
_MAC_RE = re.compile(r'^[0-9A-F]{12}$')
def _normalize_mac(mac: str) -> str: def _normalize_mac(mac: str) -> str:
"""Normalize MAC address: remove colons/hyphens, uppercase.""" """Normalize MAC address: remove colons/hyphens, uppercase. Returns empty string if invalid."""
return mac.replace(":", "").replace("-", "").upper() normalized = mac.replace(":", "").replace("-", "").upper()
if not _MAC_RE.match(normalized):
return ""
return normalized
def load_personal_macs_from_env() -> None: def load_personal_macs_from_env() -> None:
@@ -380,6 +387,9 @@ def load_personal_macs_from_env() -> None:
with personal_lock: with personal_lock:
for mac in macs: for mac in macs:
normalized = _normalize_mac(mac) normalized = _normalize_mac(mac)
if not normalized: # Skip invalid MACs
logging.warning(f"Skipping invalid MAC from NET_ALERTER_PERSONAL_MACS: {mac}")
continue
personal_devices.add(normalized) personal_devices.add(normalized)
logging.info(f"Added personal device MAC: {normalized}") logging.info(f"Added personal device MAC: {normalized}")
@@ -425,6 +435,9 @@ def is_personal_device(mac: str) -> bool:
""" """
# Normalize MAC # Normalize MAC
mac_normalized = _normalize_mac(mac) mac_normalized = _normalize_mac(mac)
if not mac_normalized: # Skip invalid MACs
return False
oui = mac_normalized[:6] oui = mac_normalized[:6]
# Check personal_devices set first (manually configured) # Check personal_devices set first (manually configured)
@@ -629,14 +642,22 @@ def on_arrival(mac: str, ip: str, hostname: str = "") -> None:
now = time.time() now = time.time()
# Lock order: known_lock, then departure_lock, then occupancy_lock # Lock order: known_lock, then departure_lock, then occupancy_lock
# Extract timers to cancel OUTSIDE all locks to avoid deadlock
timers_to_cancel = []
with departure_lock:
if mac in departure_timers:
timers_to_cancel.append(departure_timers.pop(mac))
# Cancel all timers BEFORE acquiring any other locks
for timer in timers_to_cancel:
timer.cancel()
with known_lock: with known_lock:
with departure_lock: with departure_lock:
# Check if device marked departing (rapid flap recovery) # Check if device marked departing (rapid flap recovery)
if mac in known_devices and known_devices[mac].get('departing'): if mac in known_devices and known_devices[mac].get('departing'):
# Device returned during departure window — cancel timer, clear flag, no alert # Device returned during departure window — cancel timer, clear flag, no alert
if mac in departure_timers:
departure_timers[mac].cancel()
del departure_timers[mac]
known_devices[mac]['departing'] = False known_devices[mac]['departing'] = False
with occupancy_lock: with occupancy_lock:
_update_occupancy_state_unlocked() _update_occupancy_state_unlocked()
@@ -644,31 +665,29 @@ def on_arrival(mac: str, ip: str, hostname: str = "") -> None:
logging.debug(f"Device {mac} re-arrived during departure window — suppressed alert") logging.debug(f"Device {mac} re-arrived during departure window — suppressed alert")
return return
# ALWAYS cancel any pending departure timer when device re-arrives # Log if we cancelled a timer
if mac in departure_timers: if timers_to_cancel:
departure_timers[mac].cancel()
del departure_timers[mac]
logging.debug(f"Cancelled departure timer for {mac} — device re-arrived before debounce expired") logging.debug(f"Cancelled departure timer for {mac} — device re-arrived before debounce expired")
if mac in last_departed_time: if mac in last_departed_time:
time_since_departure = now - last_departed_time[mac] time_since_departure = now - last_departed_time[mac]
if time_since_departure < 300: # 5 min if time_since_departure < 300: # 5 min
logging.debug(f"Suppressing re-arrival alert for {mac} (departed {int(time_since_departure)}s ago)") logging.debug(f"Suppressing re-arrival alert for {mac} (departed {int(time_since_departure)}s ago)")
# Re-add to known_devices to track presence # Re-add to known_devices to track presence
if mac not in known_devices: if mac not in known_devices:
vendor = lookup_oui(mac) vendor = lookup_oui(mac)
known_devices[mac] = { known_devices[mac] = {
'ip': ip, 'ip': ip,
'hostname': hostname, 'hostname': hostname,
'vendor': vendor, 'vendor': vendor,
'first_seen': now, 'first_seen': now,
'last_seen': now 'last_seen': now
} }
else: else:
known_devices[mac]['last_seen'] = now known_devices[mac]['last_seen'] = now
with occupancy_lock: with occupancy_lock:
_update_occupancy_state_unlocked() _update_occupancy_state_unlocked()
return return
# Still inside known_lock, but not inside departure_lock now # Still inside known_lock, but not inside departure_lock now
if mac in known_devices: if mac in known_devices:
@@ -732,11 +751,18 @@ def on_departure(mac: str) -> None:
known_devices[mac]['departing'] = True known_devices[mac]['departing'] = True
# Start 15-minute debounce timer # Start 15-minute debounce timer
# Extract timer to cancel outside of lock to avoid deadlock
timer_to_cancel = None
with departure_lock: with departure_lock:
# Cancel any existing timer for this MAC # Cancel any existing timer for this MAC
if mac in departure_timers: if mac in departure_timers:
departure_timers[mac].cancel() timer_to_cancel = departure_timers.pop(mac)
# Cancel outside lock
if timer_to_cancel:
timer_to_cancel.cancel()
with departure_lock:
def send_departure_alert(): def send_departure_alert():
"""Callback to send alert after debounce expires.""" """Callback to send alert after debounce expires."""
# Lock order: known_lock, then departure_lock, then occupancy_lock # Lock order: known_lock, then departure_lock, then occupancy_lock