[nft PATCH 1/3] nft: use own allocation function

[nft PATCH 1/3] nft: use own allocation function

[Arturo Borrero Gonzalez]

In the current setup, nft (the frontend object) is using the xzalloc() function
from libnftables, which does not makes sense, as this is typically an internal
helper function.

In order to don't use this public libnftables symbol (a later patch just
removes it), let's introduce a new allocation function in the nft frontend.
This results in a bit of code duplication, but given the simplicity of the code,
I don't think it's a big deal.

Other possible approach would be to have xzalloc() become part of libnftables
public API, but that is a much worse scenario I think.

Signed-off-by: Arturo Borrero Gonzalez <arturo@netfilter.org>
---
 src/main.c |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/src/main.c b/src/main.c
index cbfd69a..d5857e8 100644
--- a/src/main.c
+++ b/src/main.c
@@ -19,9 +19,24 @@
 #include <sys/types.h>
 
 #include <nftables/libnftables.h>
+#include <nftables.h>
 #include <utils.h>
 #include <cli.h>
 
+void *xzalloc(size_t size)
+{
+	void *ptr;
+
+	ptr = malloc(size);
+	if (ptr == NULL) {
+		fprintf(stderr, "%s:%u: Memory allocation failure\n",
+			__FILE__, __LINE__);
+		exit(NFT_EXIT_NOMEM);
+	}
+	memset(ptr, 0, size);
+	return ptr;
+}
+
 static struct nft_ctx *nft;
 
 enum opt_vals {

[nft PATCH 2/3] libnftables: reallocate definition of nft_print() and nft_gmp_print()

[Arturo Borrero Gonzalez]

They are not part of the libnftables library API, they are not public symbols,
so it doesn't not make sense to have them there. Move the two functions to a
different source file.

Signed-off-by: Arturo Borrero Gonzalez <arturo@netfilter.org>
---
 src/libnftables.c |   27 ---------------------------
 src/utils.c       |   26 ++++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 27 deletions(-)

diff --git a/src/libnftables.c b/src/libnftables.c
index dccb8ab..f2cd267 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -507,30 +507,3 @@ err:
 		cache_release(&nft->cache);
 	return rc;
 }
-
-int nft_print(struct output_ctx *octx, const char *fmt, ...)
-{
-	int ret;
-	va_list arg;
-
-	va_start(arg, fmt);
-	ret = vfprintf(octx->output_fp, fmt, arg);
-	va_end(arg);
-	fflush(octx->output_fp);
-
-	return ret;
-}
-
-int nft_gmp_print(struct output_ctx *octx, const char *fmt, ...)
-{
-	int ret;
-	va_list arg;
-
-	va_start(arg, fmt);
-	ret = gmp_vfprintf(octx->output_fp, fmt, arg);
-	va_end(arg);
-	fflush(octx->output_fp);
-
-	return ret;
-}
-
diff --git a/src/utils.c b/src/utils.c
index 47f5b79..69e8344 100644
--- a/src/utils.c
+++ b/src/utils.c
@@ -90,3 +90,29 @@ void xstrunescape(const char *in, char *out)
 	}
 	out[k++] = '\0';
 }
+
+int nft_print(struct output_ctx *octx, const char *fmt, ...)
+{
+	int ret;
+	va_list arg;
+
+	va_start(arg, fmt);
+	ret = vfprintf(octx->output_fp, fmt, arg);
+	va_end(arg);
+	fflush(octx->output_fp);
+
+	return ret;
+}
+
+int nft_gmp_print(struct output_ctx *octx, const char *fmt, ...)
+{
+	int ret;
+	va_list arg;
+
+	va_start(arg, fmt);
+	ret = gmp_vfprintf(octx->output_fp, fmt, arg);
+	va_end(arg);
+	fflush(octx->output_fp);
+
+	return ret;
+}

Re: [nft PATCH 2/3] libnftables: reallocate definition of nft_print() and nft_gmp_print()

[Arturo Borrero Gonzalez]

On 6/27/19 12:50 PM, Arturo Borrero Gonzalez wrote:
> They are not part of the libnftables library API, they are not public symbols,
> so it doesn't not make sense to have them there. Move the two functions to a
> different source file.
> 
> Signed-off-by: Arturo Borrero Gonzalez <arturo@netfilter.org>
> ---
>  src/libnftables.c |   27 ---------------------------
>  src/utils.c       |   26 ++++++++++++++++++++++++++
>  2 files changed, 26 insertions(+), 27 deletions(-)
> 

This patch is probably not required, it does not affect how visible the symbols
of the library are.

Drop it or apply it, I'm fine either way.

Re: [nft PATCH 2/3] libnftables: reallocate definition of nft_print() and nft_gmp_print()

[Florian Westphal]

Arturo Borrero Gonzalez <arturo@netfilter.org> wrote:
> On 6/27/19 12:50 PM, Arturo Borrero Gonzalez wrote:
> > They are not part of the libnftables library API, they are not public symbols,
> > so it doesn't not make sense to have them there. Move the two functions to a
> > different source file.
> > 
> > Signed-off-by: Arturo Borrero Gonzalez <arturo@netfilter.org>
> > ---
> >  src/libnftables.c |   27 ---------------------------
> >  src/utils.c       |   26 ++++++++++++++++++++++++++
> >  2 files changed, 26 insertions(+), 27 deletions(-)
> > 
> 
> This patch is probably not required, it does not affect how visible the symbols
> of the library are.
> 
> Drop it or apply it, I'm fine either way.

I'd be inclinded to skip because of the nft_ prefix (and removing that
adds useless code churn), provided patch 3 is accepted of course.

Re: [nft PATCH 2/3] libnftables: reallocate definition of nft_print() and nft_gmp_print()

[Pablo Neira Ayuso]

On Thu, Jun 27, 2019 at 12:50:11PM +0200, Arturo Borrero Gonzalez wrote:
> They are not part of the libnftables library API, they are not public symbols,
> so it doesn't not make sense to have them there. Move the two functions to a
> different source file.

Probably move them to src/print.c ?

I like the idea that only the API is placed in libnftables.c.

Thanks.

[nft PATCH 3/3] libnftables: export public symbols only

[Arturo Borrero Gonzalez]

Export public symbols (the library API functions) instead of all symbols in
the library.

This patch introduces the required macros to manage the visibility attributes
(mostly copied from libnftnl.git) and also marks each symbol as exported when
they need to be public. Also, introduce a .map file for proper symbol
versioning.

Previous to this patch, libnftables public symbols were:

% dpkg-gensymbols -q -plibnftables -v0.9.1 -O -esrc/.libs/libnftables.so.1 | wc -l
527

With this patch, libnftables symbols are:

% dpkg-gensymbols -q -plibnftables -v0.9.1 -O -esrc/.libs/libnftables.so.1
libnftables.so.1 libnftables #MINVER#
 nft_ctx_add_include_path@Base 0.9.1
 nft_ctx_buffer_error@Base 0.9.1
 nft_ctx_buffer_output@Base 0.9.1
 nft_ctx_clear_include_paths@Base 0.9.1
 nft_ctx_free@Base 0.9.1
 nft_ctx_get_dry_run@Base 0.9.1
 nft_ctx_get_error_buffer@Base 0.9.1
 nft_ctx_get_output_buffer@Base 0.9.1
 nft_ctx_new@Base 0.9.1
 nft_ctx_output_get_debug@Base 0.9.1
 nft_ctx_output_get_flags@Base 0.9.1
 nft_ctx_output_set_debug@Base 0.9.1
 nft_ctx_output_set_flags@Base 0.9.1
 nft_ctx_set_dry_run@Base 0.9.1
 nft_ctx_set_error@Base 0.9.1
 nft_ctx_set_output@Base 0.9.1
 nft_ctx_unbuffer_error@Base 0.9.1
 nft_ctx_unbuffer_output@Base 0.9.1
 nft_run_cmd_from_buffer@Base 0.9.1
 nft_run_cmd_from_filename@Base 0.9.1

Signed-off-by: Arturo Borrero Gonzalez <arturo@netfilter.org>
---
 configure.ac          |    5 +++++
 include/utils.h       |    8 ++++++++
 m4/gcc4_visibility.m4 |   21 +++++++++++++++++++++
 src/Makefile.am       |    8 +++++---
 src/libnftables.c     |   20 ++++++++++++++++++++
 src/libnftables.map   |   25 +++++++++++++++++++++++++
 6 files changed, 84 insertions(+), 3 deletions(-)
 create mode 100644 m4/gcc4_visibility.m4
 create mode 100644 src/libnftables.map

diff --git a/configure.ac b/configure.ac
index b71268e..26a9cb7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -44,6 +44,11 @@ fi
 
 AM_PROG_AR
 AM_PROG_LIBTOOL
+LT_INIT
+AM_PROG_CC_C_O
+AC_EXEEXT
+AC_DISABLE_STATIC
+CHECK_GCC_FVISIBILITY
 
 AS_IF([test "x$enable_man_doc" = "xyes"], [
        AC_CHECK_PROG(A2X, [a2x], [a2x], [no])
diff --git a/include/utils.h b/include/utils.h
index e791523..647e8bb 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -11,6 +11,14 @@
 #include <list.h>
 #include <gmputil.h>
 
+#include "config.h"
+#ifdef HAVE_VISIBILITY_HIDDEN
+#       define __visible        __attribute__((visibility("default")))
+#       define EXPORT_SYMBOL(x) typeof(x) (x) __visible;
+#else
+#       define EXPORT_SYMBOL
+#endif
+
 #define BITS_PER_BYTE	8
 
 #define pr_debug(fmt, arg...) printf(fmt, ##arg)
diff --git a/m4/gcc4_visibility.m4 b/m4/gcc4_visibility.m4
new file mode 100644
index 0000000..214d3f3
--- /dev/null
+++ b/m4/gcc4_visibility.m4
@@ -0,0 +1,21 @@
+
+# GCC 4.x -fvisibility=hidden
+
+AC_DEFUN([CHECK_GCC_FVISIBILITY], [
+	AC_LANG_PUSH([C])
+	saved_CFLAGS="$CFLAGS"
+	CFLAGS="$saved_CFLAGS -fvisibility=hidden"
+	AC_CACHE_CHECK([whether compiler accepts -fvisibility=hidden],
+	  [ac_cv_fvisibility_hidden], AC_COMPILE_IFELSE(
+		[AC_LANG_SOURCE()],
+		[ac_cv_fvisibility_hidden=yes],
+		[ac_cv_fvisibility_hidden=no]
+	))
+	if test "$ac_cv_fvisibility_hidden" = "yes"; then
+		AC_DEFINE([HAVE_VISIBILITY_HIDDEN], [1],
+			[True if compiler supports -fvisibility=hidden])
+		AC_SUBST([GCC_FVISIBILITY_HIDDEN], [-fvisibility=hidden])
+	fi
+	CFLAGS="$saved_CFLAGS"
+	AC_LANG_POP([C])
+])
diff --git a/src/Makefile.am b/src/Makefile.am
index fd64175..f97a354 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -19,7 +19,7 @@ AM_CFLAGS = -Wall								\
 	    -Wdeclaration-after-statement -Wsign-compare -Winit-self		\
 	    -Wformat-nonliteral -Wformat-security -Wmissing-format-attribute	\
 	    -Wcast-align -Wundef -Wbad-function-cast				\
-	    -Waggregate-return -Wunused -Wwrite-strings
+	    -Waggregate-return -Wunused -Wwrite-strings ${GCC_FVISIBILITY_HIDDEN}
 
 
 AM_YFLAGS = -d
@@ -62,7 +62,8 @@ libnftables_la_SOURCES =			\
 		nfnl_osf.c			\
 		tcpopt.c			\
 		socket.c			\
-		libnftables.c
+		libnftables.c			\
+		libnftables.map
 
 # yacc and lex generate dirty code
 noinst_LTLIBRARIES = libparser.la
@@ -76,7 +77,8 @@ libparser_la_CFLAGS = ${AM_CFLAGS} \
 		      -Wno-redundant-decls
 
 libnftables_la_LIBADD = ${LIBMNL_LIBS} ${LIBNFTNL_LIBS} libparser.la
-libnftables_la_LDFLAGS = -version-info ${libnftables_LIBVERSION}
+libnftables_la_LDFLAGS = -version-info ${libnftables_LIBVERSION} \
+			 --version-script=$(srcdir)/libnftables.map
 
 if BUILD_MINIGMP
 noinst_LTLIBRARIES += libminigmp.la
diff --git a/src/libnftables.c b/src/libnftables.c
index f2cd267..2f77a77 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -109,6 +109,7 @@ static void nft_exit(void)
 	mark_table_exit();
 }
 
+EXPORT_SYMBOL(nft_ctx_add_include_path);
 int nft_ctx_add_include_path(struct nft_ctx *ctx, const char *path)
 {
 	char **tmp;
@@ -127,6 +128,7 @@ int nft_ctx_add_include_path(struct nft_ctx *ctx, const char *path)
 	return 0;
 }
 
+EXPORT_SYMBOL(nft_ctx_clear_include_paths);
 void nft_ctx_clear_include_paths(struct nft_ctx *ctx)
 {
 	while (ctx->num_include_paths)
@@ -141,6 +143,7 @@ static void nft_ctx_netlink_init(struct nft_ctx *ctx)
 	ctx->nf_sock = nft_mnl_socket_open();
 }
 
+EXPORT_SYMBOL(nft_ctx_new);
 struct nft_ctx *nft_ctx_new(uint32_t flags)
 {
 	struct nft_ctx *ctx;
@@ -226,21 +229,25 @@ static int exit_cookie(struct cookie *cookie)
 	return 0;
 }
 
+EXPORT_SYMBOL(nft_ctx_buffer_output);
 int nft_ctx_buffer_output(struct nft_ctx *ctx)
 {
 	return init_cookie(&ctx->output.output_cookie);
 }
 
+EXPORT_SYMBOL(nft_ctx_unbuffer_output);
 int nft_ctx_unbuffer_output(struct nft_ctx *ctx)
 {
 	return exit_cookie(&ctx->output.output_cookie);
 }
 
+EXPORT_SYMBOL(nft_ctx_buffer_error);
 int nft_ctx_buffer_error(struct nft_ctx *ctx)
 {
 	return init_cookie(&ctx->output.error_cookie);
 }
 
+EXPORT_SYMBOL(nft_ctx_unbuffer_error);
 int nft_ctx_unbuffer_error(struct nft_ctx *ctx)
 {
 	return exit_cookie(&ctx->output.error_cookie);
@@ -262,16 +269,19 @@ static const char *get_cookie_buffer(struct cookie *cookie)
 	return cookie->buf;
 }
 
+EXPORT_SYMBOL(nft_ctx_get_output_buffer);
 const char *nft_ctx_get_output_buffer(struct nft_ctx *ctx)
 {
 	return get_cookie_buffer(&ctx->output.output_cookie);
 }
 
+EXPORT_SYMBOL(nft_ctx_get_error_buffer);
 const char *nft_ctx_get_error_buffer(struct nft_ctx *ctx)
 {
 	return get_cookie_buffer(&ctx->output.error_cookie);
 }
 
+EXPORT_SYMBOL(nft_ctx_free);
 void nft_ctx_free(struct nft_ctx *ctx)
 {
 	if (ctx->nf_sock)
@@ -287,6 +297,7 @@ void nft_ctx_free(struct nft_ctx *ctx)
 	nft_exit();
 }
 
+EXPORT_SYMBOL(nft_ctx_set_output);
 FILE *nft_ctx_set_output(struct nft_ctx *ctx, FILE *fp)
 {
 	FILE *old = ctx->output.output_fp;
@@ -299,6 +310,7 @@ FILE *nft_ctx_set_output(struct nft_ctx *ctx, FILE *fp)
 	return old;
 }
 
+EXPORT_SYMBOL(nft_ctx_set_error);
 FILE *nft_ctx_set_error(struct nft_ctx *ctx, FILE *fp)
 {
 	FILE *old = ctx->output.error_fp;
@@ -311,30 +323,36 @@ FILE *nft_ctx_set_error(struct nft_ctx *ctx, FILE *fp)
 	return old;
 }
 
+EXPORT_SYMBOL(nft_ctx_get_dry_run);
 bool nft_ctx_get_dry_run(struct nft_ctx *ctx)
 {
 	return ctx->check;
 }
 
+EXPORT_SYMBOL(nft_ctx_set_dry_run);
 void nft_ctx_set_dry_run(struct nft_ctx *ctx, bool dry)
 {
 	ctx->check = dry;
 }
 
+EXPORT_SYMBOL(nft_ctx_output_get_flags);
 unsigned int nft_ctx_output_get_flags(struct nft_ctx *ctx)
 {
 	return ctx->output.flags;
 }
 
+EXPORT_SYMBOL(nft_ctx_output_set_flags);
 void nft_ctx_output_set_flags(struct nft_ctx *ctx, unsigned int flags)
 {
 	ctx->output.flags = flags;
 }
 
+EXPORT_SYMBOL(nft_ctx_output_get_debug);
 unsigned int nft_ctx_output_get_debug(struct nft_ctx *ctx)
 {
 	return ctx->debug_mask;
 }
+EXPORT_SYMBOL(nft_ctx_output_set_debug);
 void nft_ctx_output_set_debug(struct nft_ctx *ctx, unsigned int mask)
 {
 	ctx->debug_mask = mask;
@@ -407,6 +425,7 @@ static int nft_evaluate(struct nft_ctx *nft, struct list_head *msgs,
 	return 0;
 }
 
+EXPORT_SYMBOL(nft_run_cmd_from_buffer);
 int nft_run_cmd_from_buffer(struct nft_ctx *nft, const char *buf)
 {
 	int rc = -EINVAL, parser_rc;
@@ -458,6 +477,7 @@ err:
 	return rc;
 }
 
+EXPORT_SYMBOL(nft_run_cmd_from_filename);
 int nft_run_cmd_from_filename(struct nft_ctx *nft, const char *filename)
 {
 	struct cmd *cmd, *next;
diff --git a/src/libnftables.map b/src/libnftables.map
new file mode 100644
index 0000000..955af38
--- /dev/null
+++ b/src/libnftables.map
@@ -0,0 +1,25 @@
+LIBNFTABLES_1 {
+global:
+  nft_ctx_add_include_path;
+  nft_ctx_clear_include_pat;
+  nft_ctx_new;
+  nft_ctx_buffer_output;
+  nft_ctx_unbuffer_output;
+  nft_ctx_buffer_error;
+  nft_ctx_unbuffer_error;
+  nft_ctx_get_output_buffer;
+  nft_ctx_get_error_buffer;
+  nft_ctx_free;
+  nft_ctx_set_output;
+  nft_ctx_set_error;
+  nft_ctx_get_dry_run;
+  nft_ctx_set_dry_run;
+  nft_ctx_output_get_flags;
+  nft_ctx_output_set_flags;
+  nft_ctx_output_get_debug;
+  nft_ctx_output_set_debug;
+  nft_run_cmd_from_buffer;
+  nft_run_cmd_from_filename;
+
+local: *;
+};

Re: [nft PATCH 3/3] libnftables: export public symbols only

[Florian Westphal]

Arturo Borrero Gonzalez <arturo@netfilter.org> wrote:
> Export public symbols (the library API functions) instead of all symbols in
> the library.

\o/

This is long overdue, thanks for doing this Arturo!

Re: [nft PATCH 1/3] nft: use own allocation function

[Florian Westphal]

Arturo Borrero Gonzalez <arturo@netfilter.org> wrote:
> In the current setup, nft (the frontend object) is using the xzalloc() function
> from libnftables, which does not makes sense, as this is typically an internal
> helper function.
> 
> In order to don't use this public libnftables symbol (a later patch just
> removes it), let's introduce a new allocation function in the nft frontend.
> This results in a bit of code duplication, but given the simplicity of the code,
> I don't think it's a big deal.
> 
> Other possible approach would be to have xzalloc() become part of libnftables
> public API, but that is a much worse scenario I think.

Agree, thus:

Acked-by: Florian Westphal <fw@strlen.de>

Re: [nft PATCH 1/3] nft: use own allocation function

[Pablo Neira Ayuso]

On Thu, Jun 27, 2019 at 12:50:00PM +0200, Arturo Borrero Gonzalez wrote:
> In the current setup, nft (the frontend object) is using the xzalloc() function
> from libnftables, which does not makes sense, as this is typically an internal
> helper function.
> 
> In order to don't use this public libnftables symbol (a later patch just
> removes it), let's introduce a new allocation function in the nft frontend.
> This results in a bit of code duplication, but given the simplicity of the code,
> I don't think it's a big deal.
> 
> Other possible approach would be to have xzalloc() become part of libnftables
> public API, but that is a much worse scenario I think.

Could you replace the call to xzalloc() in main.c by calloc()?

Also check for the error if calloc() fails, then print error and
exit().

Just like other error path in main.c

Thanks!