From a5f04b62705b054b28bcdeebc2f7e2e5784e3b33 Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Thu, 27 Oct 2022 11:15:49 +0200 Subject: [PATCH] cat_sep: Make fully vararg and do explicit runtime type checks Using positional and vararg arguments for BIFs, it's not possible to do proper runtime type checking on them as discussed in #2425. The bifcl produced code unconditionally attempts to convert the positional arguments to StringVals, but nothing ever type checks them. Instead of improving the vararg support in Zeek script and bifcl, align cat_sep() with fmt() in making it fully vararg and do implement type checks by hand. With this change, passing wrong types for the separator and default argument isn't a fatal error anymore and the error messages are also more descriptive. It's a bit of a crutch working around varargs limitations. Fixes #2425 --- NEWS | 3 ++ src/zeek.bif | 45 ++++++++++++++----- testing/btest/Baseline/bifs.cat/out | 1 + .../Baseline/bifs.cat_sep_errors-2/.stderr | 2 + .../Baseline/bifs.cat_sep_errors-3/.stderr | 2 + .../Baseline/bifs.cat_sep_errors-4/.stderr | 2 + .../Baseline/bifs.cat_sep_errors-5/.stderr | 2 + .../Baseline/bifs.cat_sep_errors/.stderr | 2 + testing/btest/bifs/cat.zeek | 4 +- testing/btest/bifs/cat_sep_errors.zeek | 33 ++++++++++++++ 10 files changed, 83 insertions(+), 13 deletions(-) create mode 100644 testing/btest/Baseline/bifs.cat_sep_errors-2/.stderr create mode 100644 testing/btest/Baseline/bifs.cat_sep_errors-3/.stderr create mode 100644 testing/btest/Baseline/bifs.cat_sep_errors-4/.stderr create mode 100644 testing/btest/Baseline/bifs.cat_sep_errors-5/.stderr create mode 100644 testing/btest/Baseline/bifs.cat_sep_errors/.stderr create mode 100644 testing/btest/bifs/cat_sep_errors.zeek diff --git a/NEWS b/NEWS index 625ee66e3f..aa4e70e28a 100644 --- a/NEWS +++ b/NEWS @@ -107,6 +107,9 @@ Changed Functionality - The parameter given to ``enum_names()`` can now be a string naming the enum type, rather than the type itself. +- Passing non-string ``sep`` and ``def`` arguments to ``cat_sep()`` isn't a + fatal error anymore. More descriptive error messages are produced, too. + Deprecated Functionality ------------------------ diff --git a/src/zeek.bif b/src/zeek.bif index 85607f2d86..11960435c2 100644 --- a/src/zeek.bif +++ b/src/zeek.bif @@ -1540,29 +1540,50 @@ function cat%(...%): string ## Concatenates all arguments, with a separator placed between each one. This ## function is similar to :zeek:id:`cat`, but places a separator between each ## given argument. If any of the variable arguments is an empty string it is -## replaced by a given default string instead. -## -## sep: The separator to place between each argument. -## -## def: The default string to use when an argument is the empty string. +## replaced by the given default string instead. This function takes at least +## two arguments. The first argument is the separator *sep* placed between +## each argument, the second argument is the default string *def* used when +## an argument is the empty string. ## ## Returns: A concatenation of all arguments with *sep* between each one and ## empty strings replaced with *def*. ## ## .. zeek:see:: cat string_cat -function cat_sep%(sep: string, def: string, ...%): string +function cat_sep%(...%): string %{ + if ( @ARGC@ < 2 ) + { + zeek::emit_builtin_error(zeek::util::fmt("cat_sep() takes at least 2 arguments, got %ld", + @ARGC@)); + return nullptr; + } + + zeek::ValPtr sep_arg = @ARG@[0]; + zeek::ValPtr def_arg = @ARG@[1]; + + if ( sep_arg->GetType()->Tag() != zeek::TYPE_STRING ) + { + zeek::emit_builtin_error(zeek::util::fmt("expected type string for separator, got %s", + zeek::type_name(sep_arg->GetType()->Tag()))); + return nullptr; + } + + if ( def_arg->GetType()->Tag() != zeek::TYPE_STRING ) + { + zeek::emit_builtin_error(zeek::util::fmt("expected type string for default, got %s", + zeek::type_name(def_arg->GetType()->Tag()))); + return nullptr; + } + + zeek::StringVal* sep = sep_arg->AsStringVal(); + zeek::StringVal* def = def_arg->AsStringVal(); + zeek::ODesc d; d.SetStyle(RAW_STYLE); - int pre_size = 0; - for ( auto i = 0u; i < @ARG@.size(); ++i ) + for ( auto i = 2u; i < @ARGC@; ++i ) { - // Skip named parameters. - if ( i < 2 ) - continue; - if ( i > 2 ) d.Add(sep->CheckString(), 0); diff --git a/testing/btest/Baseline/bifs.cat/out b/testing/btest/Baseline/bifs.cat/out index 1685af393d..5ceffa7e7a 100644 --- a/testing/btest/Baseline/bifs.cat/out +++ b/testing/btest/Baseline/bifs.cat/out @@ -4,4 +4,5 @@ foo3T 3T foo|3|T + |3|T diff --git a/testing/btest/Baseline/bifs.cat_sep_errors-2/.stderr b/testing/btest/Baseline/bifs.cat_sep_errors-2/.stderr new file mode 100644 index 0000000000..13a40bc019 --- /dev/null +++ b/testing/btest/Baseline/bifs.cat_sep_errors-2/.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. +error in <...>/cat_sep_errors.zeek, line 3: cat_sep() takes at least 2 arguments, got 1 (cat_sep(sep)) diff --git a/testing/btest/Baseline/bifs.cat_sep_errors-3/.stderr b/testing/btest/Baseline/bifs.cat_sep_errors-3/.stderr new file mode 100644 index 0000000000..c19b73c3da --- /dev/null +++ b/testing/btest/Baseline/bifs.cat_sep_errors-3/.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. +error in <...>/cat_sep_errors.zeek, line 4: expected type string for default, got count (cat_sep(sep, 1)) diff --git a/testing/btest/Baseline/bifs.cat_sep_errors-4/.stderr b/testing/btest/Baseline/bifs.cat_sep_errors-4/.stderr new file mode 100644 index 0000000000..6416ec7c35 --- /dev/null +++ b/testing/btest/Baseline/bifs.cat_sep_errors-4/.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. +error in <...>/cat_sep_errors.zeek, line 4: expected type string for separator, got count (cat_sep(1, default)) diff --git a/testing/btest/Baseline/bifs.cat_sep_errors-5/.stderr b/testing/btest/Baseline/bifs.cat_sep_errors-5/.stderr new file mode 100644 index 0000000000..53fbad9616 --- /dev/null +++ b/testing/btest/Baseline/bifs.cat_sep_errors-5/.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. +error in <...>/cat_sep_errors.zeek, line 3: expected type string for separator, got record (cat_sep([$a=1], default)) diff --git a/testing/btest/Baseline/bifs.cat_sep_errors/.stderr b/testing/btest/Baseline/bifs.cat_sep_errors/.stderr new file mode 100644 index 0000000000..f3813297fe --- /dev/null +++ b/testing/btest/Baseline/bifs.cat_sep_errors/.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. +error in <...>/cat_sep_errors.zeek, line 6: cat_sep() takes at least 2 arguments, got 0 (cat_sep()) diff --git a/testing/btest/bifs/cat.zeek b/testing/btest/bifs/cat.zeek index 5540ebf106..e2b48a5225 100644 --- a/testing/btest/bifs/cat.zeek +++ b/testing/btest/bifs/cat.zeek @@ -17,6 +17,8 @@ event zeek_init() print cat_sep("|", "", a, b, c); print cat_sep("|", ""); - + + print cat_sep("|", "", ""); + print cat_sep("|", "", "", b, c); } diff --git a/testing/btest/bifs/cat_sep_errors.zeek b/testing/btest/bifs/cat_sep_errors.zeek new file mode 100644 index 0000000000..b743381c7e --- /dev/null +++ b/testing/btest/bifs/cat_sep_errors.zeek @@ -0,0 +1,33 @@ +# @TEST-DOC: Runtime errors calling cat_sep() +# @TEST-EXEC: zeek -b %INPUT +# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff .stderr +event zeek_init() + { + print cat_sep(); + } + +@TEST-START-NEXT +event zeek_init() + { + print cat_sep("sep"); + } + +@TEST-START-NEXT +# bad types 1 +event zeek_init() + { + print cat_sep("sep", 1); + } + +@TEST-START-NEXT +# bad types 2 +event zeek_init() + { + print cat_sep(1, "default"); + } + +@TEST-START-NEXT +event zeek_init() + { + print cat_sep([$a=1], "default"); + }