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:
+55
-29
@@ -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
|
||||||
|
|||||||
Reference in New Issue
Block a user