Improve error messages from to_addr and to_subnet BIFs

Related to GH-514

src/IPAddr.cc

@@ -242,30 +242,39 @@ IPPrefix::IPPrefix(const in6_addr& in6, uint8_t length)
 	prefix.Mask(this->length);
 	}
 
-IPPrefix::IPPrefix(const IPAddr& addr, uint8_t length, bool len_is_v6_relative)
+bool IPAddr::CheckPrefixLength(uint8_t length, bool len_is_v6_relative) const
-	: prefix(addr)
 	{
-	if ( prefix.GetFamily() == IPv4 && ! len_is_v6_relative )
+	if ( GetFamily() == IPv4 && ! len_is_v6_relative )
 		{
 		if ( length > 32 )
-			{
+			return false;
-			reporter->Error("Bad IPAddr(v4) IPPrefix length : %d", length);
-			this->length = 0;
-			}
-		else
-			this->length = length + 96;
 		}
 
 	else
 		{
 		if ( length > 128 )
-			{
+			return false;
-			reporter->Error("Bad IPAddr(v6) IPPrefix length : %d", length);
-			this->length = 0;
 		}
+
+	return true;
+	}
+
+IPPrefix::IPPrefix(const IPAddr& addr, uint8_t length, bool len_is_v6_relative)
+	: prefix(addr)
+	{
+	if ( prefix.CheckPrefixLength(length, len_is_v6_relative) )
+		{
+		if ( prefix.GetFamily() == IPv4 && ! len_is_v6_relative )
+			this->length = length + 96;
 		else
 			this->length = length;
 		}
+	else
+		{
+		auto vstr = prefix.GetFamily() == IPv4 ? "v4" : "v6";
+		reporter->Error("Bad IPAddr(%s) IPPrefix length : %d", vstr, length);
+		this->length = 0;
+		}
 
 	prefix.Mask(this->length);
 	}
@@ -281,3 +290,28 @@ string IPPrefix::AsString() const
 
 	return prefix.AsString() +"/" + l;
 	}
+
+bool IPPrefix::ConvertString(const char* text, IPPrefix* result)
+	{
+	string s(text);
+	size_t slash_loc = s.find('/');
+
+	if ( slash_loc == string::npos )
+		return false;
+
+	auto ip_str = s.substr(0, slash_loc);
+	auto len = atoi(s.substr(slash_loc + 1).data());
+
+	in6_addr tmp;
+
+	if ( ! IPAddr::ConvertString(ip_str.data(), &tmp) )
+		return false;
+
+	auto ip = IPAddr(tmp);
+
+	if ( ! ip.CheckPrefixLength(len) )
+		return false;
+
+	*result = IPPrefix(ip, len);
+	return true;
+	}

src/IPAddr.h

@@ -366,6 +366,21 @@ public:
 
 	unsigned int MemoryAllocation() const { return padded_sizeof(*this); }
 
+	/**
+	 * Check if an IP prefix length would be valid against this IP address.
+	 *
+	 * @param length the IP prefix length to check
+	 *
+	 * @param len_is_v6_relative whether the length is relative to the full
+	 * IPv6 address length (e.g. since IPv4 addrs are internally stored
+	 * in v4-to-v6-mapped format, this parameter disambiguates whether
+	 * a the length is in the usual 32-bit space for IPv4 or the full
+	 * 128-bit space of IPv6 address.
+	 *
+	 * @return whether the prefix length is valid.
+	 */
+	bool CheckPrefixLength(uint8_t length, bool len_is_v6_relative = false) const;
+
 	/**
 	 * Converts an IPv4 or IPv6 string into a network address structure
 	 * (IPv6 or v4-to-v6-mapping in network bytes order).
@@ -659,6 +674,28 @@ public:
 		return ! ( net1 <= net2 );
 		}
 
+	/**
+	 * Converts an IPv4 or IPv6 prefix string into a network address prefix structure.
+	 *
+	 * @param s the IPv4 or IPv6 prefix string to convert (ASCII, NUL-terminated).
+	 *
+	 * @param result buffer that the caller supplies to store the result.
+	 *
+	 * @return whether the conversion was successful.
+	 */
+	static bool ConvertString(const char* s, IPPrefix* result);
+
+	/**
+	 * @param s the IPv4 or IPv6 prefix string to convert (ASCII, NUL-terminated).
+	 *
+	 * @return whether the string is a valid IP address prefix
+	 */
+	static bool IsValid(const char* s)
+		{
+		IPPrefix tmp;
+		return ConvertString(s, &tmp);
+		}
+
 private:
 	IPAddr prefix;	// We store it as an address with the non-prefix bits masked out via Mask().
 	uint8_t length;	// The bit length of the prefix relative to full IPv6 addr.

src/Val.cc

@@ -863,19 +863,10 @@ Val* AddrVal::DoClone(CloneState* state)
 
 SubNetVal::SubNetVal(const char* text) : Val(TYPE_SUBNET)
 	{
-	string s(text);
-	size_t slash_loc = s.find('/');
-
-	if ( slash_loc == string::npos )
-		{
-		reporter->Error("Bad string in SubNetVal ctor: %s", text);
 	val.subnet_val = new IPPrefix();
-		}
+
-	else
+	if ( ! IPPrefix::ConvertString(text, val.subnet_val) )
-		{
+		reporter->Error("Bad string in SubNetVal ctor: %s", text);
-		val.subnet_val = new IPPrefix(s.substr(0, slash_loc),
-		                              atoi(s.substr(slash_loc + 1).c_str()));
-		}
 	}
 
 SubNetVal::SubNetVal(const char* text, int width) : Val(TYPE_SUBNET)

src/zeek.bif

@@ -2421,7 +2421,17 @@ function count_to_port%(num: count, proto: transport_proto%): port
 function to_addr%(ip: string%): addr
 	%{
 	char* s = ip->AsString()->Render();
-	Val* ret = new AddrVal(s);
+	Val* ret = nullptr;
+	in6_addr tmp;
+
+	if ( IPAddr::ConvertString(s, &tmp) )
+		ret = new AddrVal(IPAddr(tmp));
+	else
+		{
+		ret = new AddrVal(IPAddr());
+		builtin_error("failed converting string to IP address", ip);
+		}
+
 	delete [] s;
 	return ret;
 	%}
@@ -2451,7 +2461,12 @@ function is_valid_ip%(ip: string%): bool
 function to_subnet%(sn: string%): subnet
 	%{
 	char* s = sn->AsString()->Render();
-	Val* ret = new SubNetVal(s);
+	IPPrefix tmp;
+
+	if ( ! IPPrefix::ConvertString(s, &tmp) )
+		builtin_error("failed converting string to IP prefix", sn);
+
+	Val* ret = new SubNetVal(tmp);
 	delete [] s;
 	return ret;
 	%}

testing/btest/Baseline/bifs.to_addr/error

@@ -1 +1 @@
-error: Bad IP address: not an IP
+error in /home/jon/pro/zeek/zeek/testing/btest/.tmp/bifs.to_addr/to_addr.zeek, line 7 and /home/jon/pro/zeek/zeek/testing/btest/.tmp/bifs.to_addr/to_addr.zeek, line 20: failed converting string to IP address (to_addr(ip) and not an IP)

testing/btest/Baseline/bifs.to_subnet/error

@@ -1 +1,3 @@
-error: Bad string in SubNetVal ctor: 10.0.0.0
+error in /home/jon/pro/zeek/zeek/testing/btest/.tmp/bifs.to_subnet/to_subnet.zeek, line 10: failed converting string to IP prefix (to_subnet(10.0.0.0) and 10.0.0.0)
+error in /home/jon/pro/zeek/zeek/testing/btest/.tmp/bifs.to_subnet/to_subnet.zeek, line 12: failed converting string to IP prefix (to_subnet(10.0.0.0/222) and 10.0.0.0/222)
+error in /home/jon/pro/zeek/zeek/testing/btest/.tmp/bifs.to_subnet/to_subnet.zeek, line 14: failed converting string to IP prefix (to_subnet(don't work) and don't work)

testing/btest/Baseline/bifs.to_subnet/output

@@ -1,3 +1,5 @@
 10.0.0.0/8, T
 2607:f8b0::/32, T
 ::/0, T
+::/0, T
+::/0, T

testing/btest/bifs/to_addr.zeek

@@ -1,6 +1,6 @@
 # @TEST-EXEC: zeek -b %INPUT >output 2>error
 # @TEST-EXEC: btest-diff output
-# @TEST-EXEC: btest-diff error
+# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff error
 
 function test_to_addr(ip: string, expect: addr)
 	{

testing/btest/bifs/to_subnet.zeek

@@ -1,6 +1,6 @@
 # @TEST-EXEC: zeek -b %INPUT >output 2>error
 # @TEST-EXEC: btest-diff output
-# @TEST-EXEC: btest-diff error
+# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff error
 
 global sn: subnet;
 sn = to_subnet("10.0.0.0/8");
@@ -9,3 +9,7 @@ sn = to_subnet("2607:f8b0::/32");
 print sn, sn == [2607:f8b0::]/32;
 sn = to_subnet("10.0.0.0");
 print sn, sn == [::]/0;
+sn = to_subnet("10.0.0.0/222");
+print sn, sn == [::]/0;
+sn = to_subnet("don't work");
+print sn, sn == [::]/0;