Merge pull request #2316 from brauner/2018-05-11/compiler_fixes

CODING_STYLE.md

@@ -662,3 +662,13 @@ rules to use them:
   #endif
   };
   ```
+
+#### Use `strlcpy()` instead of `strncpy()`
+
+When copying strings always use `strlcpy()` instead of `strncpy()`. The
+advantage of `strlcpy()` is that it will always append a `\0` byte to the
+string.
+
+Unless you have a valid reason to accept truncation you must check whether
+truncation has occurred, treat it as an error, and handle the error
+appropriately.

configure.ac

@@ -620,6 +620,10 @@ AC_CHECK_FUNCS([prlimit64],
 	AM_CONDITIONAL(HAVE_PRLIMIT64, true)
 	AC_DEFINE(HAVE_PRLIMIT64,1,[Have prlimit64]),
 	AM_CONDITIONAL(HAVE_PRLIMIT64, false))
+AC_CHECK_FUNCS([strlcpy],
+	AM_CONDITIONAL(HAVE_STRLCPY, true)
+	AC_DEFINE(HAVE_STRLCPY,1,[Have strlcpy]),
+	AM_CONDITIONAL(HAVE_STRLCPY, false))
 
 # Check for some libraries
 AX_PTHREAD

src/include/strlcpy.c

+/* liblxcapi
+ *
+ * Copyright © 2018 Christian Brauner <christian@brauner.io>.
+ * Copyright © 2018 Canonical Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2, as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * This function has been copied from musl.
+ */
+
+#include <limits.h>
+#include <stdint.h>
+#include <string.h>
+
+#define ALIGN (sizeof(size_t) - 1)
+#define ONES ((size_t)-1 / UCHAR_MAX)
+#define HIGHS (ONES * (UCHAR_MAX / 2 + 1))
+#define HASZERO(x) (((x)-ONES) & ~(x)&HIGHS)
+
+size_t strlcpy(char *d, const char *s, size_t n)
+{
+	char *d0 = d;
+	size_t *wd;
+	const size_t *ws;
+
+	if (!n--)
+		goto finish;
+
+	if (((uintptr_t)s & ALIGN) == ((uintptr_t)d & ALIGN)) {
+		for (; ((uintptr_t)s & ALIGN) && n && (*d = *s); n--, s++, d++)
+			;
+		if (n && *s) {
+			wd = (void *)d;
+			ws = (const void *)s;
+			for (; n >= sizeof(size_t) && !HASZERO(*ws);
+			     n -= sizeof(size_t), ws++, wd++)
+				*wd = *ws;
+			d = (void *)wd;
+			s = (const void *)ws;
+		}
+	}
+
+	for (; n && (*d = *s); n--, s++, d++)
+		;
+
+	*d = 0;
+
+finish:
+	return d - d0 + strlen(s);
+}

src/include/strlcpy.h

+/* liblxcapi
+ *
+ * Copyright © 2018 Christian Brauner <christian@brauner.io>.
+ * Copyright © 2018 Canonical Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2, as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * This function has been copied from musl.
+ */
+
+#ifndef _STRLCPY_H
+#define _STRLCPY_H
+
+#include <stdio.h>
+
+extern size_t strlcpy(char *, const char *, size_t);
+
+#endif

src/lxc/Makefile.am

@@ -151,6 +151,10 @@ liblxc_la_SOURCES += ../include/getline.c ../include/getline.h
 endif
 endif
 
+if !HAVE_STRLCPY
+liblxc_la_SOURCES += ../include/strlcpy.c ../include/strlcpy.h
+endif
+
 AM_CFLAGS=-DLXCROOTFSMOUNT=\"$(LXCROOTFSMOUNT)\" \
 	-DLXCPATH=\"$(LXCPATH)\" \
 	-DLXC_GLOBAL_CONF=\"$(LXC_GLOBAL_CONF)\" \
@@ -300,6 +304,10 @@ init_lxc_static_SOURCES += ../include/getline.c
 endif
 endif
 
+if !HAVE_STRLCPY
+init_lxc_static_SOURCES += ../include/strlcpy.c ../include/strlcpy.h
+endif
+
 init_lxc_static_LDFLAGS = -all-static
 init_lxc_static_LDADD = @CAP_LIBS@
 init_lxc_static_CFLAGS = $(AM_CFLAGS) -DNO_LXC_CONF

src/lxc/af_unix.c

@@ -36,6 +36,10 @@
 #include "log.h"
 #include "utils.h"
 
+#ifndef HAVE_STRLCPY
+#include "include/strlcpy.h"
+#endif
+
 lxc_log_define(lxc_af_unix, lxc);
 
 int lxc_abstract_unix_open(const char *path, int type, int flags)
@@ -63,8 +67,9 @@ int lxc_abstract_unix_open(const char *path, int type, int flags)
 		errno = ENAMETOOLONG;
 		return -1;
 	}
-	/* addr.sun_path[0] has already been set to 0 by memset() */
-	strncpy(&addr.sun_path[1], &path[1], len);
+
+	/* do not enforce \0-termination */
+	memcpy(&addr.sun_path[1], &path[1], len);
 
 	ret = bind(fd, (struct sockaddr *)&addr,
 		   offsetof(struct sockaddr_un, sun_path) + len + 1);
@@ -116,8 +121,9 @@ int lxc_abstract_unix_connect(const char *path)
 		errno = ENAMETOOLONG;
 		return -1;
 	}
-	/* addr.sun_path[0] has already been set to 0 by memset() */
-	strncpy(&addr.sun_path[1], &path[1], strlen(&path[1]));
+
+	/* do not enforce \0-termination */
+	memcpy(&addr.sun_path[1], &path[1], len);
 
 	ret = connect(fd, (struct sockaddr *)&addr,
 		      offsetof(struct sockaddr_un, sun_path) + len + 1);

src/lxc/criu.c

@@ -51,6 +51,10 @@
 #include <mntent.h>
 #endif
 
+#ifndef HAVE_STRLCPY
+#include "include/strlcpy.h"
+#endif
+
 #define CRIU_VERSION		"2.0"
 
 #define CRIU_GITID_VERSION	"2.0"
@@ -536,6 +540,7 @@ static void exec_criu(struct criu_opts *opts)
 		argv = m;
 
 		lxc_list_for_each(it, &opts->c->lxc_conf->network) {
+			size_t retlen;
 			char eth[128], *veth;
 			char *fmt;
 			struct lxc_netdev *n = it->elem;
@@ -552,9 +557,9 @@ static void exec_criu(struct criu_opts *opts)
 			}
 
 			if (n->name[0] != '\0') {
-				if (strlen(n->name) >= sizeof(eth))
+				retlen = strlcpy(eth, n->name, sizeof(eth));
+				if (retlen >= sizeof(eth))
 					goto err;
-				strncpy(eth, n->name, sizeof(eth));
 			} else {
 				ret = snprintf(eth, sizeof(eth), "eth%d", netnr);
 				if (ret < 0 || ret >= sizeof(eth))

src/lxc/log.c

@@ -46,6 +46,10 @@
 #include "utils.h"
 #include "lxccontainer.h"
 
+#ifndef HAVE_STRLCPY
+#include "include/strlcpy.h"
+#endif
+
 /* We're logging in seconds and nanoseconds. Assuming that the underlying
  * datatype is currently at maximum a 64bit integer, we have a date string that
  * is of maximum length (2^64 - 1) * 2 = (21 + 21) = 42.
@@ -690,8 +694,8 @@ extern const char *lxc_log_get_file(void)
 
 extern void lxc_log_set_prefix(const char *prefix)
 {
-	strncpy(log_prefix, prefix, sizeof(log_prefix));
-	log_prefix[sizeof(log_prefix) - 1] = 0;
+	/* We don't care if thte prefix is truncated. */
+	(void)strlcpy(log_prefix, prefix, sizeof(log_prefix));
 }
 
 extern const char *lxc_log_get_prefix(void)

src/lxc/lxccontainer.c

@@ -87,6 +87,10 @@
 #include <mntent.h>
 #endif
 
+#ifndef HAVE_STRLCPY
+#include "include/strlcpy.h"
+#endif
+
 /* Define faccessat() if missing from the C library */
 #ifndef HAVE_FACCESSAT
 static int faccessat(int __fd, const char *__file, int __type, int __flag)
@@ -728,7 +732,7 @@ static void push_arg(char ***argp, char *arg, int *nargs)
 
 static char **split_init_cmd(const char *incmd)
 {
-	size_t len;
+	size_t len, retlen;
 	char *copy, *p;
 	char **argv;
 	int nargs = 0;
@@ -739,8 +743,10 @@ static char **split_init_cmd(const char *incmd)
 
 	len = strlen(incmd) + 1;
 	copy = alloca(len);
-	strncpy(copy, incmd, len);
-	copy[len - 1] = '\0';
+	retlen = strlcpy(copy, incmd, len);
+	if (retlen >= len) {
+		return NULL;
+	}
 
 	do {
 		argv = malloc(sizeof(char *));

src/lxc/monitor.c

@@ -49,6 +49,10 @@
 #include "state.h"
 #include "utils.h"
 
+#ifndef HAVE_STRLCPY
+#include "include/strlcpy.h"
+#endif
+
 lxc_log_define(lxc_monitor, lxc);
 
 /* routines used by monitor publishers (containers) */
@@ -131,9 +135,8 @@ void lxc_monitor_send_state(const char *name, lxc_state_t state,
 			    const char *lxcpath)
 {
 	struct lxc_msg msg = {.type = lxc_msg_state, .value = state};
-	strncpy(msg.name, name, sizeof(msg.name));
-	msg.name[sizeof(msg.name) - 1] = 0;
 
+	(void)strlcpy(msg.name, name, sizeof(msg.name));
 	lxc_monitor_fifo_send(&msg, lxcpath);
 }
 
@@ -141,9 +144,8 @@ void lxc_monitor_send_exit_code(const char *name, int exit_code,
 				const char *lxcpath)
 {
 	struct lxc_msg msg = {.type = lxc_msg_exit_code, .value = exit_code};
-	strncpy(msg.name, name, sizeof(msg.name));
-	msg.name[sizeof(msg.name) - 1] = 0;
 
+	(void)strlcpy(msg.name, name, sizeof(msg.name));
 	lxc_monitor_fifo_send(&msg, lxcpath);
 }
 

src/lxc/network.c

@@ -59,6 +59,10 @@
 #include <../include/ifaddrs.h>
 #endif
 
+#ifndef HAVE_STRLCPY
+#include "include/strlcpy.h"
+#endif
+
 #ifndef IFLA_LINKMODE
 #define IFLA_LINKMODE 17
 #endif
@@ -1894,6 +1898,7 @@ static int lxc_ovs_attach_bridge(const char *bridge, const char *nic)
 int lxc_bridge_attach(const char *bridge, const char *ifname)
 {
 	int err, fd, index;
+	size_t retlen;
 	struct ifreq ifr;
 
 	if (strlen(ifname) >= IFNAMSIZ)
@@ -1910,7 +1915,10 @@ int lxc_bridge_attach(const char *bridge, const char *ifname)
 	if (fd < 0)
 		return -errno;
 
-	strncpy(ifr.ifr_name, bridge, IFNAMSIZ - 1);
+	retlen = strlcpy(ifr.ifr_name, bridge, IFNAMSIZ);
+	if (retlen >= IFNAMSIZ)
+		return -E2BIG;
+
 	ifr.ifr_name[IFNAMSIZ - 1] = '\0';
 	ifr.ifr_ifindex = index;
 	err = ioctl(fd, SIOCBRADDIF, &ifr);
@@ -2113,6 +2121,7 @@ static int lxc_create_network_unpriv_exec(const char *lxcpath, const char *lxcna
 
 	if (child == 0) {
 		int ret;
+		size_t retlen;
 		char pidstr[LXC_NUMSTRLEN64];
 
 		close(pipefd[0]);
@@ -2127,9 +2136,13 @@ static int lxc_create_network_unpriv_exec(const char *lxcpath, const char *lxcna
 		}
 
 		if (netdev->link[0] != '\0')
-			strncpy(netdev_link, netdev->link, IFNAMSIZ - 1);
+			retlen = strlcpy(netdev_link, netdev->link, IFNAMSIZ);
 		else
-			strncpy(netdev_link, "none", IFNAMSIZ - 1);
+			retlen = strlcpy(netdev_link, "none", IFNAMSIZ);
+		if (retlen >= IFNAMSIZ) {
+			SYSERROR("Invalid network device name");
+			_exit(EXIT_FAILURE);
+		}
 
 		ret = snprintf(pidstr, LXC_NUMSTRLEN64, "%d", pid);
 		if (ret < 0 || ret >= LXC_NUMSTRLEN64)

src/lxc/start.c

@@ -89,6 +89,10 @@
 #include "terminal.h"
 #include "utils.h"
 
+#ifndef HAVE_STRLCPY
+#include "include/strlcpy.h"
+#endif
+
 lxc_log_define(lxc_start, lxc);
 
 extern void mod_all_rdeps(struct lxc_container *c, bool inc);
@@ -410,6 +414,7 @@ static int signal_handler(int fd, uint32_t events, void *data,
 int lxc_serve_state_clients(const char *name, struct lxc_handler *handler,
 			    lxc_state_t state)
 {
+	size_t retlen;
 	ssize_t ret;
 	struct lxc_list *cur, *next;
 	struct lxc_state_client *client;
@@ -427,8 +432,9 @@ int lxc_serve_state_clients(const char *name, struct lxc_handler *handler,
 		return 0;
 	}
 
-	strncpy(msg.name, name, sizeof(msg.name));
-	msg.name[sizeof(msg.name) - 1] = 0;
+	retlen = strlcpy(msg.name, name, sizeof(msg.name));
+	if (retlen >= sizeof(msg.name))
+		return -E2BIG;
 
 	lxc_list_for_each_safe(cur, &handler->conf->state_clients, next) {
 		client = cur->elem;

src/lxc/storage/btrfs.c

@@ -42,6 +42,10 @@
 #include "storage.h"
 #include "utils.h"
 
+#ifndef HAVE_STRLCPY
+#include "include/strlcpy.h"
+#endif
+
 lxc_log_define(btrfs, lxc);
 
 /*
@@ -223,6 +227,7 @@ int btrfs_umount(struct lxc_storage *bdev)
 static int btrfs_subvolume_create(const char *path)
 {
 	int ret, saved_errno;
+	size_t retlen;
 	struct btrfs_ioctl_vol_args args;
 	char *p, *newfull;
 	int fd = -1;
@@ -248,8 +253,9 @@ static int btrfs_subvolume_create(const char *path)
 	}
 
 	memset(&args, 0, sizeof(args));
-	strncpy(args.name, p + 1, BTRFS_SUBVOL_NAME_MAX);
-	args.name[BTRFS_SUBVOL_NAME_MAX - 1] = 0;
+	retlen = strlcpy(args.name, p + 1, BTRFS_SUBVOL_NAME_MAX);
+	if (retlen >= BTRFS_SUBVOL_NAME_MAX)
+		return -E2BIG;
 
 	ret = ioctl(fd, BTRFS_IOC_SUBVOL_CREATE, &args);
 	saved_errno = errno;
@@ -303,6 +309,7 @@ out:
 
 int btrfs_snapshot(const char *orig, const char *new)
 {
+	size_t retlen;
 	struct btrfs_ioctl_vol_args_v2 args;
 	char *newdir, *newname;
 	char *newfull = NULL;
@@ -328,9 +335,9 @@ int btrfs_snapshot(const char *orig, const char *new)
 		goto out;
 
 	memset(&args, 0, sizeof(args));
-	args.fd = fd;
-	strncpy(args.name, newname, BTRFS_SUBVOL_NAME_MAX);
-	args.name[BTRFS_SUBVOL_NAME_MAX - 1] = 0;
+	retlen = strlcpy(args.name, newname, BTRFS_SUBVOL_NAME_MAX);
+	if (retlen >= BTRFS_SUBVOL_NAME_MAX)
+		goto out;
 
 	ret = ioctl(fddst, BTRFS_IOC_SNAP_CREATE_V2, &args);
 	saved_errno = errno;
@@ -498,6 +505,7 @@ bool btrfs_create_snapshot(struct lxc_conf *conf, struct lxc_storage *orig,
 static int btrfs_do_destroy_subvol(const char *path)
 {
 	int ret, fd = -1;
+	size_t retlen;
 	struct btrfs_ioctl_vol_args  args;
 	char *p, *newfull = strdup(path);
 
@@ -522,8 +530,12 @@ static int btrfs_do_destroy_subvol(const char *path)
 	}
 
 	memset(&args, 0, sizeof(args));
-	strncpy(args.name, p+1, BTRFS_SUBVOL_NAME_MAX);
-	args.name[BTRFS_SUBVOL_NAME_MAX-1] = 0;
+	retlen = strlcpy(args.name, p+1, BTRFS_SUBVOL_NAME_MAX);
+	if (retlen >= BTRFS_SUBVOL_NAME_MAX) {
+		free(newfull);
+		return -E2BIG;
+	}
+
 	ret = ioctl(fd, BTRFS_IOC_SNAP_DESTROY, &args);
 	INFO("btrfs: snapshot destroy ioctl returned %d for %s", ret, path);
 	if (ret < 0 && errno == EPERM)
@@ -577,21 +589,25 @@ static bool update_tree_node(struct mytree_node *n, u64 id, u64 parent,
 {
 	if (id)
 		n->objid = id;
+
 	if (parent)
 		n->parentid = parent;
+
 	if (name) {
 		n->name = malloc(name_len + 1);
 		if (!n->name)
 			return false;
-		strncpy(n->name, name, name_len);
-		n->name[name_len] = '\0';
+
+		strcpy(n->name, name);
 	}
+
 	if (dirname) {
 		n->dirname = malloc(strlen(dirname) + 1);
 		if (!n->dirname) {
 			free(n->name);
 			return false;
 		}
+
 		strcpy(n->dirname, dirname);
 	}
 	return true;

src/lxc/storage/storage_utils.c

@@ -46,6 +46,10 @@
 #include "storage_utils.h"
 #include "utils.h"
 
+#ifndef HAVE_STRLCPY
+#include "include/strlcpy.h"
+#endif
+
 #ifndef BLKGETSIZE64
 #define BLKGETSIZE64 _IOR(0x12, 114, size_t)
 #endif
@@ -85,8 +89,13 @@ char *dir_new_path(char *src, const char *oldname, const char *name,
 	}
 
 	while ((p2 = strstr(src, oldname)) != NULL) {
+		size_t retlen;
+
 		/* copy text up to oldname */
-		strncpy(p, src, p2 - src);
+		retlen = strlcpy(p, src, p2 - src);
+		if (retlen >= p2 - src)
+			return NULL;
+
 		/* move target pointer (p) */
 		p += p2 - src;
 		/* print new name in place of oldname */
@@ -94,6 +103,7 @@ char *dir_new_path(char *src, const char *oldname, const char *name,
 		/* move src to end of oldname */
 		src = p2 + l2;
 	}
+
 	/* copy the rest of src */
 	sprintf(p, "%s", src);
 	return ret;