From 7092db6318c66eefc0fedad96cb10c732b239aa0 Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Mon, 28 Apr 2025 14:36:11 +0200 Subject: [PATCH] broker/Data/data_to_val: Fail on vectors/lists with holes Instead of simply removing holes from vectors or lists when converting from Val to Broker format, error out as the receiver has no chance to reconstruct where the hole might have been. We could encode holes with broker::none, but this will put unnecessary burden on language bindings and users due to the potential optionality. Think a std::vector that technically needs to be a std::vector> to represent optional elements properly. Closes #3045 --- NEWS | 6 ++ src/broker/Data.cc | 12 ++- .../..manager..stderr | 3 + .../..manager..stdout | 6 ++ .../..worker-1..stderr | 2 + .../..worker-1..stdout | 4 + .../cluster/generic/publish-vec-hole.zeek | 92 +++++++++++++++++++ 7 files changed, 121 insertions(+), 4 deletions(-) create mode 100644 testing/btest/Baseline/cluster.generic.publish-vec-hole/..manager..stderr create mode 100644 testing/btest/Baseline/cluster.generic.publish-vec-hole/..manager..stdout create mode 100644 testing/btest/Baseline/cluster.generic.publish-vec-hole/..worker-1..stderr create mode 100644 testing/btest/Baseline/cluster.generic.publish-vec-hole/..worker-1..stdout create mode 100644 testing/btest/cluster/generic/publish-vec-hole.zeek diff --git a/NEWS b/NEWS index 2b92732fd7..b981118f80 100644 --- a/NEWS +++ b/NEWS @@ -17,6 +17,12 @@ New Functionality Changed Functionality --------------------- +- Publishing remote events with vector arguments that contain holes is now + rejected. The receiver side never had a chance to figure out where these + holes would have been. There's a chance this breaks scripts that accidentally + published vectors with holes. A reporter error is produced at runtime when + serialization of vectors with holes is attempted. + Removed Functionality --------------------- diff --git a/src/broker/Data.cc b/src/broker/Data.cc index e9a6492b88..850f88219b 100644 --- a/src/broker/Data.cc +++ b/src/broker/Data.cc @@ -841,8 +841,10 @@ std::optional val_to_data(const Val* v) { for ( auto i = 0u; i < vec->Size(); ++i ) { auto item_val = vec->ValAt(i); - if ( ! item_val ) - continue; + if ( ! item_val ) { + reporter->Error("serialization of vectors with holes is unsupported"); + return std::nullopt; + } auto item = val_to_data(item_val.get()); @@ -864,8 +866,10 @@ std::optional val_to_data(const Val* v) { for ( auto i = 0; i < list->Length(); ++i ) { const auto& item_val = list->Idx(i); - if ( ! item_val ) - continue; + if ( ! item_val ) { + reporter->Error("serialization of lists with holes is unsupported"); + return std::nullopt; + } auto item = val_to_data(item_val.get()); diff --git a/testing/btest/Baseline/cluster.generic.publish-vec-hole/..manager..stderr b/testing/btest/Baseline/cluster.generic.publish-vec-hole/..manager..stderr new file mode 100644 index 0000000000..0138046c28 --- /dev/null +++ b/testing/btest/Baseline/cluster.generic.publish-vec-hole/..manager..stderr @@ -0,0 +1,3 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +error in ../manager.zeek, line 12: serialization of vectors with holes is unsupported +received termination signal diff --git a/testing/btest/Baseline/cluster.generic.publish-vec-hole/..manager..stdout b/testing/btest/Baseline/cluster.generic.publish-vec-hole/..manager..stdout new file mode 100644 index 0000000000..15e138e0b6 --- /dev/null +++ b/testing/btest/Baseline/cluster.generic.publish-vec-hole/..manager..stdout @@ -0,0 +1,6 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +node_up, worker-1 +got pong, with, [1, 2, 3], 3 +got pong, with, [1, 2, 3], 3 +got pong, with, [4, 5, 6], 3 +got pong, with, [4, 5, 6], 3 diff --git a/testing/btest/Baseline/cluster.generic.publish-vec-hole/..worker-1..stderr b/testing/btest/Baseline/cluster.generic.publish-vec-hole/..worker-1..stderr new file mode 100644 index 0000000000..e3f6131b1d --- /dev/null +++ b/testing/btest/Baseline/cluster.generic.publish-vec-hole/..worker-1..stderr @@ -0,0 +1,2 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +received termination signal diff --git a/testing/btest/Baseline/cluster.generic.publish-vec-hole/..worker-1..stdout b/testing/btest/Baseline/cluster.generic.publish-vec-hole/..worker-1..stdout new file mode 100644 index 0000000000..99b4863cdd --- /dev/null +++ b/testing/btest/Baseline/cluster.generic.publish-vec-hole/..worker-1..stdout @@ -0,0 +1,4 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +got ping, vector of count, [1, 2, 3], 3 +got ping, vector of count, [4, 5, 6], 3 +got finish! diff --git a/testing/btest/cluster/generic/publish-vec-hole.zeek b/testing/btest/cluster/generic/publish-vec-hole.zeek new file mode 100644 index 0000000000..971b29e46d --- /dev/null +++ b/testing/btest/cluster/generic/publish-vec-hole.zeek @@ -0,0 +1,92 @@ +# @TEST-DOC: Attempt to send an event with holes. It should fail. +# +# @TEST-REQUIRES: have-zeromq +# +# @TEST-PORT: XPUB_PORT +# @TEST-PORT: XSUB_PORT +# @TEST-PORT: LOG_PULL_PORT +# +# @TEST-EXEC: cp $FILES/zeromq/cluster-layout-no-logger.zeek cluster-layout.zeek +# @TEST-EXEC: cp $FILES/zeromq/test-bootstrap.zeek zeromq-test-bootstrap.zeek +# +# @TEST-EXEC: zeek -b --parse-only common.zeek manager.zeek worker.zeek +# +# @TEST-EXEC: btest-bg-run manager "ZEEKPATH=$ZEEKPATH:.. && CLUSTER_NODE=manager zeek -b ../manager.zeek" +# @TEST-EXEC: btest-bg-run worker-1 "ZEEKPATH=$ZEEKPATH:.. && CLUSTER_NODE=worker-1 zeek -b ../worker.zeek" +# +# @TEST-EXEC: btest-bg-wait 30 +# @TEST-EXEC: btest-diff ./manager/.stderr +# @TEST-EXEC: btest-diff ./manager/.stdout +# @TEST-EXEC: btest-diff ./worker-1/.stdout +# @TEST-EXEC: btest-diff ./worker-1/.stderr + +# @TEST-START-FILE common.zeek +@load ./zeromq-test-bootstrap.zeek + +redef Log::default_rotation_interval = 0sec; + +global finish: event() &is_used; +global ping: event(v: vector of count) &is_used; +global pong: event(v: vector of count) &is_used; +# @TEST-END-FILE + +# @TEST-START-FILE manager.zeek +@load ./common.zeek + +event send_pings() + { + local v1 = vector(1, 2, 3); + + assert Cluster::publish(Cluster::worker_topic, ping, v1); + + # Publish with a vector with a hole, fails! + local v2 = vector(1); + v2[2] = 3; + assert ! Cluster::publish(Cluster::worker_topic, ping, v2); + local v3 = vector(4, 5, 6); + assert Cluster::publish(Cluster::worker_topic, ping, v3); + } + +global pongs = 0; + +event pong(v: vector of count) + { + ++pongs; + print "got pong", "with", v, |v|; + + # Two of the three pings go through, the worker sends 2 pongs + # for each ping, so stop after 4. + if ( pongs == 4 ) + Cluster::publish(Cluster::worker_topic, finish); + } + +event Cluster::node_up(name: string, id: string) + { + print "node_up", name; + event send_pings(); + } + +event Cluster::node_down(name: string, id: string) + { + terminate(); + } +# @TEST-END-FILE + + +# @TEST-START-FILE worker.zeek +@load ./common.zeek + +event ping(v: vector of count) + { + print "got ping", type_name(v), cat(v), |v|; + Cluster::publish(Cluster::manager_topic, pong, v); + local e = Cluster::make_event(pong, v); + Cluster::publish(Cluster::manager_topic, e); + } + +event finish() + { + print "got finish!"; + terminate(); + } +# @TEST-END-FILE