nsenter: correctly handle newgidmap path for rootless containers

After quite a bit of debugging, I found that previous versions of this
patchset did not include newgidmap in a rootless setting. Fix this by
passing it whenever group mappings are applied, and also providing some
better checking for try_mapping_tool. This commit also includes some
stylistic improvements.

Signed-off-by: Aleksa Sarai <asarai@suse.de>

libcontainer/container_linux.go

@@ -1709,10 +1709,12 @@ func (c *linuxContainer) bootstrapData(cloneFlags uintptr, nsMaps map[configs.Na
 	if !joinExistingUser {
 		// write uid mappings
 		if len(c.config.UidMappings) > 0 {
+			if c.config.Rootless && c.newuidmapPath != "" {
 				r.AddData(&Bytemsg{
 					Type:  UidmapPathAttr,
 					Value: []byte(c.newuidmapPath),
 				})
+			}
 			b, err := encodeIDMapping(c.config.UidMappings)
 			if err != nil {
 				return nil, err
@@ -1733,12 +1735,14 @@ func (c *linuxContainer) bootstrapData(cloneFlags uintptr, nsMaps map[configs.Na
 				Type:  GidmapAttr,
 				Value: b,
 			})
-			// The following only applies if we are root.
-			if !c.config.Rootless {
+			if c.config.Rootless && c.newgidmapPath != "" {
 				r.AddData(&Bytemsg{
 					Type:  GidmapPathAttr,
 					Value: []byte(c.newgidmapPath),
 				})
+			}
+			// The following only applies if we are root.
+			if !c.config.Rootless {
 				// check if we have CAP_SETGID to setgroup properly
 				pid, err := capability.NewPid(os.Getpid())
 				if err != nil {

libcontainer/nsenter/nsexec.c

@@ -67,21 +67,27 @@ struct clone_t {
 
 struct nlconfig_t {
 	char *data;
+
+	/* Process settings. */
 	uint32_t cloneflags;
+	char *oom_score_adj;
+	size_t oom_score_adj_len;
+
+	/* User namespace settings.*/
 	char *uidmap;
 	size_t uidmap_len;
 	char *gidmap;
 	size_t gidmap_len;
+	char *namespaces;
+	size_t namespaces_len;
+	uint8_t is_setgroup;
+
+	/* Rootless container settings.*/
+	uint8_t is_rootless;
 	char *uidmappath;
 	size_t uidmappath_len;
 	char *gidmappath;
 	size_t gidmappath_len;
-	char *namespaces;
-	size_t namespaces_len;
-	uint8_t is_setgroup;
-	uint8_t is_rootless;
-	char *oom_score_adj;
-	size_t oom_score_adj_len;
 };
 
 /*
@@ -202,18 +208,29 @@ static void update_setgroups(int pid, enum policy_t setgroup)
 
 static int try_mapping_tool(const char *app, int pid, char *map, size_t map_len)
 {
-	int child = fork();
+	int child;
+
+	/*
+	 * If @app is NULL, execvp will segfault. Just check it here and bail (if
+	 * we're in this path, the caller is already getting desparate and there
+	 * isn't a backup to this failing). This usually would be a configuration
+	 * or programming issue.
+	 */
+	if (!app)
+		bail("mapping tool not present");
+
+	child = fork();
 	if (child < 0)
 		bail("failed to fork");
 
-	if (child == 0) {
+	if (!child) {
 #define MAX_ARGV 20
 		char *argv[MAX_ARGV];
 		char pid_fmt[16];
 		int argc = 0;
 		char *next;
 
-		snprintf (pid_fmt, 16, "%d", pid);
+		snprintf(pid_fmt, 16, "%d", pid);
 
 		argv[argc++] = (char *) app;
 		argv[argc++] = pid_fmt;
@@ -228,16 +245,18 @@ static int try_mapping_tool(const char *app, int pid, char *map, size_t map_len)
 				break;
 			}
 			argv[argc++] = map;
-			next = strpbrk (map, "\n ");
+			next = strpbrk(map, "\n ");
 			if (next == NULL)
 				break;
 			*next++ = '\0';
 			map = next + strspn(next, "\n ");
 		}
-		execvp (app, argv);
-	}
-	else {
+
+		execvp(app, argv);
+		bail("failed to execvp");
+	} else {
 		int status;
+
 		while (true) {
 			if (waitpid(child, &status, 0) < 0) {
 				if (errno == EINTR)
@@ -248,6 +267,7 @@ static int try_mapping_tool(const char *app, int pid, char *map, size_t map_len)
 				return WEXITSTATUS(status);
 		}
 	}
+
 	return -1;
 }
 
@@ -257,9 +277,9 @@ static void update_uidmap(const char *path, int pid, char *map, size_t map_len)
 		return;
 
 	if (write_file(map, map_len, "/proc/%d/uid_map", pid) < 0) {
-		if(errno != EPERM)
-			bail("failed to update /proc/%d/gid_map", pid);
-		if (try_mapping_tool (path, pid, map, map_len))
+		if (errno != EPERM)
+			bail("failed to update /proc/%d/uid_map", pid);
+		if (try_mapping_tool(path, pid, map, map_len))
 			bail("failed to use newuid map on %d", pid);
 	}
 }
@@ -270,9 +290,9 @@ static void update_gidmap(const char *path, int pid, char *map, size_t map_len)
 		return;
 
 	if (write_file(map, map_len, "/proc/%d/gid_map", pid) < 0) {
-		if(errno != EPERM)
+		if (errno != EPERM)
 			bail("failed to update /proc/%d/gid_map", pid);
-		if (try_mapping_tool (path, pid, map, map_len))
+		if (try_mapping_tool(path, pid, map, map_len))
 			bail("failed to use newgid map on %d", pid);
 	}
 }