[ulogd2 PATCH 0/5] Format string fixes

[ulogd2 PATCH 0/5] Format string fixes

[Jeremy Sowden]

The first patch adds gcc's `format` attribute to the `ulogd_log` logging
function and the following four patches fix the bugs revealed by it.

Jeremy Sowden (5):
  include: add `format` attribute to `__ulogd_log` declaration
  ulogd: remove empty log-line
  ulogd: fix order of log arguments
  input: UNIXSOCK: correct format specifiers
  output: IPFIX: correct format specifiers

 include/ulogd/ulogd.h                |  5 +++--
 input/packet/ulogd_inppkt_UNIXSOCK.c | 11 ++++++-----
 output/ipfix/ulogd_output_IPFIX.c    |  9 +++++----
 src/ulogd.c                          |  3 +--
 4 files changed, 15 insertions(+), 13 deletions(-)

-- 
2.33.0

[ulogd2 PATCH 1/5] include: add `format` attribute to `__ulogd_log` declaration

[Jeremy Sowden]

`__ulogd_log` takes a printf-style format string and matching arguments.
Add the gcc `format` attribute to its declaration in order to allow the
compiler to type-check the function arguments against the specifiers in
the format string.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 include/ulogd/ulogd.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/ulogd/ulogd.h b/include/ulogd/ulogd.h
index 1636a8caa854..a487c8e70e37 100644
--- a/include/ulogd/ulogd.h
+++ b/include/ulogd/ulogd.h
@@ -299,8 +299,9 @@ void ulogd_register_plugin(struct ulogd_plugin *me);
 /* allocate a new ulogd_key */
 struct ulogd_key *alloc_ret(const uint16_t type, const char*);
 
-/* write a message to the daemons' logfile */
-void __ulogd_log(int level, char *file, int line, const char *message, ...);
+/* write a message to the daemon's logfile */
+void __ulogd_log(int level, char *file, int line, const char *message, ...)
+	__attribute__((format(printf, 4, 5)));
 /* macro for logging including filename and line number */
 #define ulogd_log(level, format, args...) \
 	__ulogd_log(level, __FILE__, __LINE__, format, ## args)
-- 
2.33.0

[ulogd2 PATCH 2/5] ulogd: remove empty log-line

[Jeremy Sowden]

There is a `strdup` at the beginning of `create_stack`.  If it fails, an
empty log-line is printed.  It's not useful, so remove it.  This is
consistent with the error-handling of the `malloc` which immediately
follows it.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 src/ulogd.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/ulogd.c b/src/ulogd.c
index 9cd64e8e19b2..a31b35592a87 100644
--- a/src/ulogd.c
+++ b/src/ulogd.c
@@ -965,7 +965,6 @@ static int create_stack(const char *option)
 		load_all_plugins();
 
 	if (!buf) {
-		ulogd_log(ULOGD_ERROR, "");
 		ret = -ENOMEM;
 		goto out_buf;
 	}
-- 
2.33.0

[ulogd2 PATCH 3/5] ulogd: fix order of log arguments

[Jeremy Sowden]

If `daemon` fails during start-up, ulogd attempts to print `errno` and
`strerror(errno)` to the log.  However, the arguments are the wrong way
round.  Swap them.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 src/ulogd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/ulogd.c b/src/ulogd.c
index a31b35592a87..97da4fc0018f 100644
--- a/src/ulogd.c
+++ b/src/ulogd.c
@@ -1569,7 +1569,7 @@ int main(int argc, char* argv[])
 	if (daemonize){
 		if (daemon(0, 0) < 0) {
 			ulogd_log(ULOGD_FATAL, "can't daemonize: %s (%d)\n",
-				  errno, strerror(errno));
+				  strerror(errno), errno);
 			warn_and_exit(daemonize);
 		}
 	}
-- 
2.33.0

[ulogd2 PATCH 4/5] input: UNIXSOCK: correct format specifiers

[Jeremy Sowden]

There are a couple of logging calls which use the wrong specifiers for
their integer arguments.  Change the specifiers to match the arguments.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 input/packet/ulogd_inppkt_UNIXSOCK.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/input/packet/ulogd_inppkt_UNIXSOCK.c b/input/packet/ulogd_inppkt_UNIXSOCK.c
index 39944bf5cdb1..86ab590073d8 100644
--- a/input/packet/ulogd_inppkt_UNIXSOCK.c
+++ b/input/packet/ulogd_inppkt_UNIXSOCK.c
@@ -18,6 +18,7 @@
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
 */
 
+#include <inttypes.h>
 #include <unistd.h>
 #include <stdlib.h>
 #include <netinet/ether.h>
@@ -632,9 +633,9 @@ static int unixsock_instance_read_cb(int fd, unsigned int what, void *param)
 		packet_sig = ntohl(unixsock_packet->marker);
 		if (packet_sig != ULOGD_SOCKET_MARK) {
 			ulogd_log(ULOGD_ERROR,
-				"ulogd2: invalid packet marked received "
-				"(read %lx, expected %lx), closing socket.\n",
-				packet_sig, ULOGD_SOCKET_MARK);
+				  "ulogd2: invalid packet marked received "
+				  "(read %" PRIx32 ", expected %" PRIx32 "), closing socket.\n",
+				  packet_sig, ULOGD_SOCKET_MARK);
 			_disconnect_client(ui);
 			return -1;
 
@@ -663,8 +664,8 @@ static int unixsock_instance_read_cb(int fd, unsigned int what, void *param)
 			}
 
 		} else {
-			ulogd_log(ULOGD_DEBUG, "  We have %d bytes, but need %d. Requesting more\n",
-					ui->unixsock_buf_avail, needed_len + sizeof(uint32_t));
+			ulogd_log(ULOGD_DEBUG, "  We have %u bytes, but need %zu. Requesting more\n",
+				  ui->unixsock_buf_avail, needed_len + sizeof(uint32_t));
 			return 0;
 		}
 
-- 
2.33.0

[ulogd2 PATCH 5/5] output: IPFIX: correct format specifiers

[Jeremy Sowden]

There are a couple of logging calls which use the wrong specifiers for
their integer arguments.  Change the specifiers to match the arguments.

Use the correct type for the variable holding the return-value of
`send(2)`.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 output/ipfix/ulogd_output_IPFIX.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/output/ipfix/ulogd_output_IPFIX.c b/output/ipfix/ulogd_output_IPFIX.c
index 5b5900363853..4863d008562e 100644
--- a/output/ipfix/ulogd_output_IPFIX.c
+++ b/output/ipfix/ulogd_output_IPFIX.c
@@ -198,7 +198,8 @@ static int send_msgs(struct ulogd_pluginstance *pi)
 	struct ipfix_priv *priv = (struct ipfix_priv *) &pi->private;
 	struct llist_head *curr, *tmp;
 	struct ipfix_msg *msg;
-	int ret = ULOGD_IRET_OK, sent;
+	int ret = ULOGD_IRET_OK;
+	ssize_t sent;
 
 	llist_for_each_prev(curr, &priv->list) {
 		msg = llist_entry(curr, struct ipfix_msg, link);
@@ -212,8 +213,8 @@ static int send_msgs(struct ulogd_pluginstance *pi)
 
 		/* TODO handle short send() for other protocols */
 		if ((size_t) sent < ipfix_msg_len(msg))
-			ulogd_log(ULOGD_ERROR, "short send: %d < %d\n",
-					sent, ipfix_msg_len(msg));
+			ulogd_log(ULOGD_ERROR, "short send: %zd < %zu\n",
+				  sent, ipfix_msg_len(msg));
 	}
 
 	llist_for_each_safe(curr, tmp, &priv->list) {
@@ -242,7 +243,7 @@ static int ipfix_ufd_cb(int fd, unsigned what, void *arg)
 			ulogd_log(ULOGD_INFO, "connection reset by peer\n");
 			ulogd_unregister_fd(&priv->ufd);
 		} else
-			ulogd_log(ULOGD_INFO, "unexpected data (%d bytes)\n", nread);
+			ulogd_log(ULOGD_INFO, "unexpected data (%zd bytes)\n", nread);
 	}
 
 	return 0;
-- 
2.33.0

Re: [ulogd2 PATCH 0/5] Format string fixes

[Pablo Neira Ayuso]

On Sun, Nov 21, 2021 at 08:41:34PM +0000, Jeremy Sowden wrote:
> The first patch adds gcc's `format` attribute to the `ulogd_log` logging
> function and the following four patches fix the bugs revealed by it.

Series applied, thanks.