[nft PATCH v3 0/3] src: use reentrant getprotobyname_r()/getprotobynumber_r()/getservbyport_r()

[nft PATCH v3 0/3] src: use reentrant getprotobyname_r()/getprotobynumber_r()/getservbyport_r()

[Thomas Haller]

Changes since version 2:

- split the patch.

- add and use defines NFT_PROTONAME_MAXSIZE, NFT_SERVNAME_MAXSIZE,
  NETDB_BUFSIZE.

- add new GPL2+ source file as a place for the wrapper functions.

Thomas Haller (3):
  nftutils: add new internal file for general utilities
  nftutils: add wrappers for getprotoby{name,number}_r(),
    getservbyport_r()
  src: use wrappers for getprotoby{name,number}_r(), getservbyport_r()

 configure.ac    |   4 ++
 src/Makefile.am |   2 +
 src/datatype.c  |  33 ++++++++--------
 src/json.c      |  22 +++++------
 src/nftutils.c  | 102 ++++++++++++++++++++++++++++++++++++++++++++++++
 src/nftutils.h  |  21 ++++++++++
 src/rule.c      |   7 ++--
 7 files changed, 161 insertions(+), 30 deletions(-)
 create mode 100644 src/nftutils.c
 create mode 100644 src/nftutils.h

-- 
2.41.0

[nft PATCH v3 1/3] nftutils: add new internal file for general utilities

[Thomas Haller]

No code there, yet.

Similar to "utils.c", it is a place for various helper functions.

But this will be GPL-2.0-or-later licensed, from the start.

"utils.c" would be a better name, but that is already taken.

The header is not under include/ directory, but placed alongside the
source files. That is, because this is internal API to the library.
It also should be included with quotes instead of angle brackets.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 src/Makefile.am | 2 ++
 src/nftutils.c  | 5 +++++
 src/nftutils.h  | 5 +++++
 3 files changed, 12 insertions(+)
 create mode 100644 src/nftutils.c
 create mode 100644 src/nftutils.h

diff --git a/src/Makefile.am b/src/Makefile.am
index ace38bd75a97..ad22a918c120 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -64,6 +64,8 @@ libnftables_la_SOURCES =			\
 		segtree.c			\
 		gmputil.c			\
 		utils.c				\
+		nftutils.c			\
+		nftutils.h			\
 		erec.c				\
 		mnl.c				\
 		iface.c				\
diff --git a/src/nftutils.c b/src/nftutils.c
new file mode 100644
index 000000000000..758283d1b650
--- /dev/null
+++ b/src/nftutils.c
@@ -0,0 +1,5 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#include <config.h>
+
+#include "nftutils.h"
diff --git a/src/nftutils.h b/src/nftutils.h
new file mode 100644
index 000000000000..9ad68d55ce47
--- /dev/null
+++ b/src/nftutils.h
@@ -0,0 +1,5 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef NFTUTILS_H
+#define NFTUTILS_H
+
+#endif /* NFTUTILS_H */
-- 
2.41.0

[nft PATCH v3 2/3] nftutils: add wrappers for getprotoby{name,number}_r(), getservbyport_r()

[Thomas Haller]

We should aim to use the thread-safe variants of getprotoby{name,number}
and getservbyport(). However, they may not be available with other libc,
so it requires a configure check. As that is cumbersome, add wrappers
that do that at one place.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 configure.ac   |  4 +++
 src/nftutils.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++
 src/nftutils.h | 16 +++++++++
 3 files changed, 117 insertions(+)

diff --git a/configure.ac b/configure.ac
index b0201ac3528e..42f0dc4cf392 100644
--- a/configure.ac
+++ b/configure.ac
@@ -108,6 +108,10 @@ AC_DEFINE([HAVE_LIBJANSSON], [1], [Define if you have libjansson])
 ])
 AM_CONDITIONAL([BUILD_JSON], [test "x$with_json" != xno])
 
+AC_CHECK_DECLS([getprotobyname_r, getprotobynumber_r, getservbyport_r], [], [], [[
+#include <netdb.h>
+]])
+
 AC_CONFIG_FILES([					\
 		Makefile				\
 		libnftables.pc				\
diff --git a/src/nftutils.c b/src/nftutils.c
index 758283d1b650..13f879ddc5c7 100644
--- a/src/nftutils.c
+++ b/src/nftutils.c
@@ -3,3 +3,100 @@
 #include <config.h>
 
 #include "nftutils.h"
+
+#include <netdb.h>
+#include <string.h>
+#include <stdint.h>
+
+/* Buffer size used for getprotobynumber_r() and similar. The manual comments
+ * that a buffer of 1024 should be sufficient "for most applications"(??), so
+ * let's double it.  It still fits reasonably on the stack, so no need to
+ * choose a smaller one. */
+#define NETDB_BUFSIZE 2048
+
+bool nft_getprotobynumber(int proto, char *out_name, size_t name_len)
+{
+	const struct protoent *result;
+
+#if HAVE_DECL_GETPROTOBYNUMBER_R
+	struct protoent result_buf;
+	char buf[NETDB_BUFSIZE];
+	int r;
+
+	r = getprotobynumber_r(proto,
+	                       &result_buf,
+	                       buf,
+	                       sizeof(buf),
+	                       (struct protoent **) &result);
+	if (r != 0 || result != &result_buf)
+		result = NULL;
+#else
+	result = getprotobynumber(proto);
+#endif
+
+	if (!result)
+		return false;
+
+	if (strlen(result->p_name) >= name_len)
+		return false;
+	strcpy(out_name, result->p_name);
+	return true;
+}
+
+int nft_getprotobyname(const char *name)
+{
+	const struct protoent *result;
+
+#if HAVE_DECL_GETPROTOBYNAME_R
+	struct protoent result_buf;
+	char buf[NETDB_BUFSIZE];
+	int r;
+
+	r = getprotobyname_r(name,
+	                     &result_buf,
+	                     buf,
+	                     sizeof(buf),
+	                     (struct protoent **) &result);
+	if (r != 0 || result != &result_buf)
+		result = NULL;
+#else
+	result = getprotobyname(name);
+#endif
+
+	if (!result)
+		return -1;
+
+	if (result->p_proto < 0 || result->p_proto > UINT8_MAX)
+		return -1;
+	return (uint8_t) result->p_proto;
+}
+
+bool nft_getservbyport(int port, const char *proto, char *out_name, size_t name_len)
+{
+	const struct servent *result;
+
+#if HAVE_DECL_GETSERVBYPORT_R
+	struct servent result_buf;
+	char buf[NETDB_BUFSIZE];
+	int r;
+
+	r = getservbyport_r(port,
+	                    proto,
+	                    &result_buf,
+	                    buf,
+	                    sizeof(buf),
+	                    (struct servent**) &result);
+	if (r != 0 || result != &result_buf)
+		result = NULL;
+#else
+	result = getservbyport(port, proto);
+#endif
+
+	if (!result)
+		return false;
+
+	if (strlen(result->s_name) >= name_len)
+		return false;
+	strcpy(out_name, result->s_name);
+	return true;
+}
diff --git a/src/nftutils.h b/src/nftutils.h
index 9ad68d55ce47..cb584b9ca32b 100644
--- a/src/nftutils.h
+++ b/src/nftutils.h
@@ -2,4 +2,20 @@
 #ifndef NFTUTILS_H
 #define NFTUTILS_H
 
+#include <stdbool.h>
+#include <stddef.h>
+
+/* The maximum buffer size for (struct protoent).p_name. It is excessively large,
+ * while still reasonably fitting on the stack. Arbitrarily chosen. */
+#define NFT_PROTONAME_MAXSIZE 1024
+
+bool nft_getprotobynumber(int number, char *out_name, size_t name_len);
+int nft_getprotobyname(const char *name);
+
+/* The maximum buffer size for (struct servent).s_name. It is excessively large,
+ * while still reasonably fitting on the stack. Arbitrarily chosen. */
+#define NFT_SERVNAME_MAXSIZE 1024
+
+bool nft_getservbyport(int port, const char *proto, char *out_name, size_t name_len);
+
 #endif /* NFTUTILS_H */
-- 
2.41.0

[nft PATCH v3 3/3] src: use wrappers for getprotoby{name,number}_r(), getservbyport_r()

[Thomas Haller]

These wrappers are thread-safe, if libc provides the reentrant versions.
Use them.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 src/datatype.c | 33 +++++++++++++++++----------------
 src/json.c     | 22 +++++++++++-----------
 src/rule.c     |  7 ++++---
 3 files changed, 32 insertions(+), 30 deletions(-)

diff --git a/src/datatype.c b/src/datatype.c
index da802a18bccd..381320eaf842 100644
--- a/src/datatype.c
+++ b/src/datatype.c
@@ -29,6 +29,7 @@
 #include <netlink.h>
 #include <json.h>
 #include <misspell.h>
+#include "nftutils.h"
 
 #include <netinet/ip_icmp.h>
 
@@ -697,12 +698,11 @@ const struct datatype ip6addr_type = {
 static void inet_protocol_type_print(const struct expr *expr,
 				      struct output_ctx *octx)
 {
-	struct protoent *p;
-
 	if (!nft_output_numeric_proto(octx)) {
-		p = getprotobynumber(mpz_get_uint8(expr->value));
-		if (p != NULL) {
-			nft_print(octx, "%s", p->p_name);
+		char name[NFT_PROTONAME_MAXSIZE];
+
+		if (nft_getprotobynumber(mpz_get_uint8(expr->value), name, sizeof(name))) {
+			nft_print(octx, "%s", name);
 			return;
 		}
 	}
@@ -711,15 +711,15 @@ static void inet_protocol_type_print(const struct expr *expr,
 
 static void inet_protocol_type_describe(struct output_ctx *octx)
 {
-	struct protoent *p;
 	uint8_t protonum;
 
 	for (protonum = 0; protonum < UINT8_MAX; protonum++) {
-		p = getprotobynumber(protonum);
-		if (!p)
+		char name[NFT_PROTONAME_MAXSIZE];
+
+		if (!nft_getprotobynumber(protonum, name, sizeof(name)))
 			continue;
 
-		nft_print(octx, "\t%-30s\t%u\n", p->p_name, protonum);
+		nft_print(octx, "\t%-30s\t%u\n", name, protonum);
 	}
 }
 
@@ -727,7 +727,6 @@ static struct error_record *inet_protocol_type_parse(struct parse_ctx *ctx,
 						     const struct expr *sym,
 						     struct expr **res)
 {
-	struct protoent *p;
 	uint8_t proto;
 	uintmax_t i;
 	char *end;
@@ -740,11 +739,13 @@ static struct error_record *inet_protocol_type_parse(struct parse_ctx *ctx,
 
 		proto = i;
 	} else {
-		p = getprotobyname(sym->identifier);
-		if (p == NULL)
+		int r;
+
+		r = nft_getprotobyname(sym->identifier);
+		if (r < 0)
 			return error(&sym->location, "Could not resolve protocol name");
 
-		proto = p->p_proto;
+		proto = r;
 	}
 
 	*res = constant_expr_alloc(&sym->location, &inet_protocol_type,
@@ -768,12 +769,12 @@ const struct datatype inet_protocol_type = {
 static void inet_service_print(const struct expr *expr, struct output_ctx *octx)
 {
 	uint16_t port = mpz_get_be16(expr->value);
-	const struct servent *s = getservbyport(port, NULL);
+	char name[NFT_SERVNAME_MAXSIZE];
 
-	if (s == NULL)
+	if (!nft_getservbyport(port, NULL, name, sizeof(name)))
 		nft_print(octx, "%hu", ntohs(port));
 	else
-		nft_print(octx, "\"%s\"", s->s_name);
+		nft_print(octx, "\"%s\"", name);
 }
 
 void inet_service_type_print(const struct expr *expr, struct output_ctx *octx)
diff --git a/src/json.c b/src/json.c
index a119dfc4f1eb..57a597bce467 100644
--- a/src/json.c
+++ b/src/json.c
@@ -15,6 +15,7 @@
 #include <netlink.h>
 #include <rule.h>
 #include <rt.h>
+#include "nftutils.h"
 
 #include <netdb.h>
 #include <netinet/icmp6.h>
@@ -297,10 +298,10 @@ static json_t *chain_print_json(const struct chain *chain)
 
 static json_t *proto_name_json(uint8_t proto)
 {
-	const struct protoent *p = getprotobynumber(proto);
+	char name[NFT_PROTONAME_MAXSIZE];
 
-	if (p)
-		return json_string(p->p_name);
+	if (nft_getprotobynumber(proto, name, sizeof(name)))
+		return json_string(name);
 	return json_integer(proto);
 }
 
@@ -1093,12 +1094,11 @@ json_t *boolean_type_json(const struct expr *expr, struct output_ctx *octx)
 json_t *inet_protocol_type_json(const struct expr *expr,
 				struct output_ctx *octx)
 {
-	struct protoent *p;
-
 	if (!nft_output_numeric_proto(octx)) {
-		p = getprotobynumber(mpz_get_uint8(expr->value));
-		if (p != NULL)
-			return json_string(p->p_name);
+		char name[NFT_PROTONAME_MAXSIZE];
+
+		if (nft_getprotobynumber(mpz_get_uint8(expr->value), name, sizeof(name)))
+			return json_string(name);
 	}
 	return integer_type_json(expr, octx);
 }
@@ -1106,13 +1106,13 @@ json_t *inet_protocol_type_json(const struct expr *expr,
 json_t *inet_service_type_json(const struct expr *expr, struct output_ctx *octx)
 {
 	uint16_t port = mpz_get_be16(expr->value);
-	const struct servent *s = NULL;
+	char name[NFT_SERVNAME_MAXSIZE];
 
 	if (!nft_output_service(octx) ||
-	    (s = getservbyport(port, NULL)) == NULL)
+	    !nft_getservbyport(port, NULL, name, sizeof(name)))
 		return json_integer(ntohs(port));
 
-	return json_string(s->s_name);
+	return json_string(name);
 }
 
 json_t *mark_type_json(const struct expr *expr, struct output_ctx *octx)
diff --git a/src/rule.c b/src/rule.c
index 99c4f0bb8b00..b59fcd3a9fa8 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -27,6 +27,7 @@
 #include <cache.h>
 #include <owner.h>
 #include <intervals.h>
+#include "nftutils.h"
 
 #include <libnftnl/common.h>
 #include <libnftnl/ruleset.h>
@@ -1666,10 +1667,10 @@ struct obj *obj_lookup_fuzzy(const char *obj_name,
 
 static void print_proto_name_proto(uint8_t l4, struct output_ctx *octx)
 {
-	const struct protoent *p = getprotobynumber(l4);
+	char name[NFT_PROTONAME_MAXSIZE];
 
-	if (p)
-		nft_print(octx, "%s", p->p_name);
+	if (nft_getprotobynumber(l4, name, sizeof(name)))
+		nft_print(octx, "%s", name);
 	else
 		nft_print(octx, "%d", l4);
 }
-- 
2.41.0

Re: [nft PATCH v3 0/3] src: use reentrant getprotobyname_r()/getprotobynumber_r()/getservbyport_r()

[Pablo Neira Ayuso]

On Fri, Aug 18, 2023 at 04:08:18PM +0200, Thomas Haller wrote:
> Changes since version 2:
> 
> - split the patch.
> 
> - add and use defines NFT_PROTONAME_MAXSIZE, NFT_SERVNAME_MAXSIZE,
>   NETDB_BUFSIZE.
> 
> - add new GPL2+ source file as a place for the wrapper functions.

Series LGTM. I would just collapse patch 1 and 2, I can do that before
applying if you like. Or you send v4 as you prefer.

Thanks.

Re: [nft PATCH v3 0/3] src: use reentrant getprotobyname_r()/getprotobynumber_r()/getservbyport_r()

[Thomas Haller]

On Fri, 2023-08-18 at 18:12 +0200, Pablo Neira Ayuso wrote:
> On Fri, Aug 18, 2023 at 04:08:18PM +0200, Thomas Haller wrote:
> > Changes since version 2:
> > 
> > - split the patch.
> > 
> > - add and use defines NFT_PROTONAME_MAXSIZE, NFT_SERVNAME_MAXSIZE,
> >   NETDB_BUFSIZE.
> > 
> > - add new GPL2+ source file as a place for the wrapper functions.
> 
> Series LGTM. I would just collapse patch 1 and 2, I can do that
> before
> applying if you like. Or you send v4 as you prefer.

Hi Pablo,


if you are OK with applying it (and mangling it first), please go
ahead.

Thank you!!
Thomas

Re: [nft PATCH v3 0/3] src: use reentrant getprotobyname_r()/getprotobynumber_r()/getservbyport_r()

[Pablo Neira Ayuso]

On Fri, Aug 18, 2023 at 07:39:31PM +0200, Thomas Haller wrote:
> On Fri, 2023-08-18 at 18:12 +0200, Pablo Neira Ayuso wrote:
> > On Fri, Aug 18, 2023 at 04:08:18PM +0200, Thomas Haller wrote:
> > > Changes since version 2:
> > > 
> > > - split the patch.
> > > 
> > > - add and use defines NFT_PROTONAME_MAXSIZE, NFT_SERVNAME_MAXSIZE,
> > >   NETDB_BUFSIZE.
> > > 
> > > - add new GPL2+ source file as a place for the wrapper functions.
> > 
> > Series LGTM. I would just collapse patch 1 and 2, I can do that
> > before
> > applying if you like. Or you send v4 as you prefer.
> 
> Hi Pablo,
> 
> if you are OK with applying it (and mangling it first), please go
> ahead.

Applied, thanks.