From 1a0ce51f38be81b15c0ce7a27c83866540f436a0 Mon Sep 17 00:00:00 2001 From: YueGuobin Date: Tue, 4 Aug 2026 23:20:58 +0800 Subject: [PATCH] marker: validate def BPF once, skip re-validation on inherited fan-out MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A marker definition validated its BPF N times — once per link in the fan-out (start_marker runs validate_bpf_syntax on every copy), spawning one tcpdump -d subprocess per link for the same expression. Definitions did not validate BPF at all; only direction was checked. Move the single validation point to the definition layer (create/update), and validate each definition's BPF on project load (dropping any that have gone invalid, like private markers). The inherited fan-out (start_marker) and def sync (update_marker) now skip validate_bpf_syntax for inherited copies, since the BPF comes from an already-validated definition. Private per-link markers still validate inline as before. uBridge still runs pcap_compile at install, so an invalid expression can never slip through. Creating a definition over N links now runs one tcpdump instead of N. --- docs/features/marker-traffic-insight.md | 6 ++ gns3server/controller/project.py | 38 ++++++++++- gns3server/controller/udp_link.py | 22 +++++-- tests/controller/test_marker.py | 85 +++++++++++++++++++++++-- 4 files changed, 139 insertions(+), 12 deletions(-) diff --git a/docs/features/marker-traffic-insight.md b/docs/features/marker-traffic-insight.md index e468e6faa..04d638282 100644 --- a/docs/features/marker-traffic-insight.md +++ b/docs/features/marker-traffic-insight.md @@ -348,6 +348,12 @@ direction relative to the capture node; see [Direction](#direction). - **Render hints are not enforced.** `color` and `highlight_duration` (milliseconds, `>= 1`) are stored on the link and never sent to uBridge; `null` lets the UI apply its own default. A partial PUT (e.g. changing only `bpf`) leaves them untouched. +- **BPF is validated once per source.** A private per-link marker validates its BPF inline + on create/update. A definition validates its BPF once at create/update (and once per + definition on project load, dropping any whose BPF has gone invalid); the inherited + fan-out to every link then skips re-validation, so creating a definition over *N* links + runs one `tcpdump -d` rather than *N*. (uBridge still runs `pcap_compile` itself at + install time, so an invalid expression can never slip through.) - **Supported node types.** A marker needs a uBridge bridge: `vpcs`, `qemu`, `docker`, `iou`, `dynamips`, `cloud` (one capable endpoint suffices). Types without a uBridge are silently skipped by the inheritance fan-out. IOU uses one shared `IOL-BRIDGE` per node but diff --git a/gns3server/controller/project.py b/gns3server/controller/project.py index 8a2eb1590..831fa1952 100644 --- a/gns3server/controller/project.py +++ b/gns3server/controller/project.py @@ -965,6 +965,21 @@ class Project: """ return self._marker_definitions + def _validate_marker_definition_bpf(self, name, bpf): + """ + Validate a marker definition's BPF once, here, so the fan-out to every + link (``_apply_def_to_all_links`` → ``inherit_marker`` → ``start_marker``) + and the per-link sync (``update_marker_definition`` → ``update_marker``) + can skip re-validation for the inherited copies — otherwise one + ``tcpdump -d`` subprocess runs per link for the same expression. A + private per-link marker still validates in ``start_marker``/``update_marker``. + """ + result = validate_bpf_syntax(bpf) + if not result.get("valid"): + raise ControllerError( + f"Marker definition '{name}': invalid BPF — {result.get('error', 'unknown error')}" + ) + def _validate_marker_definition_direction(self, name, direction): """ Reject tx/rx on a marker definition: a definition fans out to every link @@ -997,6 +1012,7 @@ class Project: f"Marker definition '{name}' already exists in this project" ) + self._validate_marker_definition_bpf(name, bpf) self._validate_marker_definition_direction(name, direction) self._marker_definitions[name] = {"bpf": bpf, "tag": tag, "direction": direction, "color": color, "highlight_duration": highlight_duration, "paused": False} await self._apply_def_to_all_links(name) @@ -1015,6 +1031,7 @@ class Project: d = self._marker_definitions[name] if bpf is not None: + self._validate_marker_definition_bpf(name, bpf) d["bpf"] = bpf if tag is not None: d["tag"] = tag @@ -1489,10 +1506,27 @@ class Project: setattr(self, key, val) # marker_definitions is loaded separately (it is not a __init__ kwarg - # nor a simple attribute — it backs a read-only property). + # nor a simple attribute — it backs a read-only property). Each BPF + # is validated once here so the inherited fan-out (start_marker) can + # skip re-validation; an invalid definition is dropped with a warning + # rather than failing the open — it could not fan out anyway. defs = project_data.get("marker_definitions") if isinstance(defs, dict): - self._marker_definitions = defs + clean_defs = {} + for def_name, d in defs.items(): + bpf = d.get("bpf") + if not bpf: + log.warning("Dropping marker definition '%s' on load: missing bpf", def_name) + continue + result = validate_bpf_syntax(bpf) + if not result.get("valid"): + log.warning( + "Dropping marker definition '%s' on load: invalid BPF (%s)", + def_name, result.get("error") + ) + continue + clean_defs[def_name] = d + self._marker_definitions = clean_defs topology = project_data["topology"] for compute in topology.get("computes", []): diff --git a/gns3server/controller/udp_link.py b/gns3server/controller/udp_link.py index 13c202751..0c5f2eeca 100644 --- a/gns3server/controller/udp_link.py +++ b/gns3server/controller/udp_link.py @@ -379,9 +379,15 @@ class UDPLink(Link): if name in self._markers: raise ControllerError(f"Marker '{name}' already exists on link {self._id}") - result = validate_bpf_syntax(bpf) - if not result.get("valid"): - raise ControllerError(f"Invalid BPF expression: {result.get('error', 'unknown error')}") + # Validate the BPF only for private per-link markers. An inherited copy + # (``inherited_from`` set) fans out from a definition whose BPF was + # already validated once at create/update (and on project load), so + # re-validating per link would spawn one ``tcpdump -d`` per link for the + # same expression. + if not inherited_from: + result = validate_bpf_syntax(bpf) + if not result.get("valid"): + raise ControllerError(f"Invalid BPF expression: {result.get('error', 'unknown error')}") if capture_node_id and not inherited_from: marker_side = self._node_by_id(capture_node_id) @@ -474,9 +480,13 @@ class UDPLink(Link): # Merge every changed field into the marker state first. if bpf is not None and bpf != marker_info["bpf"]: - result = validate_bpf_syntax(bpf) - if not result.get("valid"): - raise ControllerError(f"Invalid BPF expression: {result.get('error', 'unknown error')}") + # An inherited marker is synced from a definition whose BPF was + # already validated at create/update (or load); re-validating per + # link is redundant. Private markers validate here as before. + if not inherited: + result = validate_bpf_syntax(bpf) + if not result.get("valid"): + raise ControllerError(f"Invalid BPF expression: {result.get('error', 'unknown error')}") marker_info["bpf"] = bpf if tag is not None: marker_info["tag"] = tag diff --git a/tests/controller/test_marker.py b/tests/controller/test_marker.py index 78e4f6467..9c13edbf8 100644 --- a/tests/controller/test_marker.py +++ b/tests/controller/test_marker.py @@ -28,6 +28,7 @@ import uuid import pytest from unittest.mock import MagicMock, patch +from contextlib import ExitStack from tests.utils import AsyncioMagicMock @@ -38,11 +39,20 @@ from gns3server.controller.controller_error import ControllerError, ControllerNo def _valid_bpf(): - """Bypass tcpdump-based BPF validation so tests don't depend on tcpdump.""" - return patch( + """Bypass tcpdump-based BPF validation so tests don't depend on tcpdump. + + Patches both namespaces that import ``validate_bpf_syntax`` by name: the + per-link ``udp_link`` (private marker create/update) and the project layer + (definition create/update/load), which is now the single validation point + for inherited copies. + """ + stack = ExitStack() + for target in ( "gns3server.controller.udp_link.validate_bpf_syntax", - return_value={"valid": True, "error": None}, - ) + "gns3server.controller.project.validate_bpf_syntax", + ): + stack.enter_context(patch(target, return_value={"valid": True, "error": None})) + return stack async def _make_link(project): @@ -438,6 +448,73 @@ async def test_apply_defs_to_new_link(project): assert new_link.markers["global-arp"]["inherited_from"] == "arp" +@pytest.mark.asyncio +async def test_create_marker_definition_validates_bpf_once(project): + # A definition validates its BPF once (project layer); the inherited fan-out + # to every link must NOT re-validate — no tcpdump subprocess per link. + with patch("gns3server.controller.project.validate_bpf_syntax", + return_value={"valid": True, "error": None}) as proj_val, \ + patch("gns3server.controller.udp_link.validate_bpf_syntax", + return_value={"valid": True, "error": None}) as link_val: + await _make_link(project) + await _make_link(project) + await project.create_marker_definition("arp", "arp") + + assert proj_val.call_count == 1 # validated once at the def layer + assert link_val.call_count == 0 # fan-out skipped per-link validation + + +@pytest.mark.asyncio +async def test_create_marker_definition_rejects_invalid_bpf(project): + + with patch("gns3server.controller.project.validate_bpf_syntax", + return_value={"valid": False, "error": "syntax error"}): + with pytest.raises(ControllerError): + await project.create_marker_definition("arp", "not a real bpf") + assert "arp" not in project.marker_definitions + + +@pytest.mark.asyncio +async def test_update_marker_definition_skips_per_link_validation(project): + # Updating a def's BPF validates once more (project); the per-link sync + # (update_marker with inherited=True) must NOT re-validate. + with patch("gns3server.controller.project.validate_bpf_syntax", + return_value={"valid": True, "error": None}) as proj_val, \ + patch("gns3server.controller.udp_link.validate_bpf_syntax", + return_value={"valid": True, "error": None}) as link_val: + await _make_link(project) + await _make_link(project) + await project.create_marker_definition("arp", "arp") + await project.update_marker_definition("arp", bpf="arp or rarp") + + assert proj_val.call_count == 2 # once on create, once on update + assert link_val.call_count == 0 # sync skipped per-link validation + + +@pytest.mark.asyncio +async def test_start_marker_skips_validation_for_inherited(project): + # An inherited marker rides an already-validated definition BPF, so + # start_marker must not call validate_bpf_syntax. + with patch("gns3server.controller.udp_link.validate_bpf_syntax", + return_value={"valid": True, "error": None}) as link_val: + link = await _make_link(project) + await link.inherit_marker("arp", {"bpf": "arp"}) + + assert link_val.call_count == 0 + assert link.markers["global-arp"]["bpf"] == "arp" + + +@pytest.mark.asyncio +async def test_start_marker_validates_for_private(project): + # A private (non-inherited) marker still validates inline. + with patch("gns3server.controller.udp_link.validate_bpf_syntax", + return_value={"valid": True, "error": None}) as link_val: + link = await _make_link(project) + await link.start_marker("icmp", "icmp") + + assert link_val.call_count == 1 + + @pytest.mark.asyncio async def test_markers_aggregation(project):