sync

commit 72ea211d57a0f235a2d7439a7e908af66f47fa10
Author: kristaps <>
Date:   Tue Feb 12 07:29:55 2019 +0000

    Sanitise group handling to handle group wheel (according to rsync, this is not
    ever remapped by name) and empty group names (not allowed, but we must accept them
    anyway).  Push most of this code into ids.c and make sure it is style(9)
    conformant.  Add TODO about performance and clarify protocol in rsync.5.

5 files changed, 82 insertions, 29 deletions

diff --git a/usr.bin/rsync/TODO.md b/usr.bin/rsync/TODO.md
index 3357ac7d864..7263e3f177f 100644
--- a/usr.bin/rsync/TODO.md
+++ b/usr.bin/rsync/TODO.md
@@ -8,6 +8,12 @@ I've included the specific security porting topics below.
 This list also does not include adding support for features (e.g., **-u** and
 so on).
 
+- Easy: speed up the uid/gid mapping/remapping with a simple table.
+  Right now, the code in 
+  [ids.c](https://github.com/kristapsdz/openrsync/blob/master/ids.c)
+  is simple, but could easily bottleneck with a large number of groups
+  and files with **-g**.
+
 - Easy: add a hashtable to `blk_find()` in
   [blocks.c](https://github.com/kristapsdz/openrsync/blob/master/blocks.c)
   for quickly looking up fast-hash matches.
diff --git a/usr.bin/rsync/extern.h b/usr.bin/rsync/extern.h
index 350351754c9..72d59a14d62 100644
--- a/usr.bin/rsync/extern.h
+++ b/usr.bin/rsync/extern.h
@@ -1,4 +1,4 @@
-/*	$Id: extern.h,v 1.6 2019/02/12 19:04:52 benno Exp $ */
+/*	$Id: extern.h,v 1.7 2019/02/12 19:13:03 benno Exp $ */
 /*
  * Copyright (c) 2019 Kristaps Dzonsons <kristaps@bsd.lv>
  *
@@ -302,10 +302,12 @@ int		  sess_stats_send(struct sess *, int);
 int		  sess_stats_recv(struct sess *, int);
 
 void		  idents_free(struct ident *, size_t);
+void		  idents_gid_assign(struct sess *, 
+			struct flist *, size_t, const struct ident *, size_t);
 void		  idents_gid_remap(struct sess *, struct ident *, size_t);
 int		  idents_gid_add(struct sess *, struct ident **, size_t *, gid_t);
-int		  idents_send(struct sess *, int, const struct ident *, size_t);
 int		  idents_recv(struct sess *, int, struct ident **, size_t *);
+int		  idents_send(struct sess *, int, const struct ident *, size_t);
 
 __END_DECLS
 
diff --git a/usr.bin/rsync/flist.c b/usr.bin/rsync/flist.c
index 980433fa424..9a42012a948 100644
--- a/usr.bin/rsync/flist.c
+++ b/usr.bin/rsync/flist.c
@@ -1,4 +1,4 @@
-/*	$Id: flist.c,v 1.8 2019/02/12 19:04:52 benno Exp $ */
+/*	$Id: flist.c,v 1.9 2019/02/12 19:13:03 benno Exp $ */
 /*
  * Copyright (c) 2019 Kristaps Dzonsons <kristaps@bsd.lv>
  *
@@ -540,7 +540,7 @@ flist_recv(struct sess *sess, int fd, struct flist **flp, size_t *sz)
 	struct flist	*fl = NULL;
 	struct flist	*ff;
 	const struct flist *fflast = NULL;
-	size_t		 i, j, flsz = 0, flmax = 0, lsz, gidsz = 0;
+	size_t		 flsz = 0, flmax = 0, lsz, gidsz = 0;
 	uint8_t		 flag;
 	char		 last[MAXPATHLEN];
 	uint64_t	 lval; /* temporary values... */
@@ -666,7 +666,6 @@ flist_recv(struct sess *sess, int fd, struct flist **flp, size_t *sz)
 			goto out;
 		}
 		LOG2(sess, "received gid list: %zu", gidsz);
-		idents_gid_remap(sess, gids, gidsz);
 	}
 
 	/* Remember to order the received list. */
@@ -677,16 +676,11 @@ flist_recv(struct sess *sess, int fd, struct flist **flp, size_t *sz)
 	*sz = flsz;
 	*flp = fl;
 
-	/* Lastly, reassign group identifiers. */
+	/* Lastly, remap and reassign group identifiers. */
 
 	if (sess->opts->preserve_gids) {
-		for (i = 0; i < flsz; i++) {
-			for (j = 0; j < gidsz; j++)
-				if ((int32_t)fl[i].st.gid == gids[j].id)
-					break;
-			assert(j < gidsz);
-			fl[i].st.gid = gids[j].mapped;
-		}
+		idents_gid_remap(sess, gids, gidsz);
+		idents_gid_assign(sess, fl, flsz, gids, gidsz);
 	}
 
 	idents_free(gids, gidsz);
diff --git a/usr.bin/rsync/ids.c b/usr.bin/rsync/ids.c
index 722e3a766c1..2ccb4cb9808 100644
--- a/usr.bin/rsync/ids.c
+++ b/usr.bin/rsync/ids.c
@@ -1,4 +1,4 @@
-/*	$Id: ids.c,v 1.2 2019/02/12 19:09:12 benno Exp $ */
+/*	$Id: ids.c,v 1.3 2019/02/12 19:13:03 benno Exp $ */
 /*
  * Copyright (c) 2019 Kristaps Dzonsons <kristaps@bsd.lv>
  *
@@ -33,7 +33,7 @@ idents_free(struct ident *p, size_t sz)
 {
 	size_t	 i;
 
-	if (NULL == p)
+	if (p == NULL)
 		return;
 	for (i = 0; i < sz; i++)
 		free(p[i].name);
@@ -41,6 +41,28 @@ idents_free(struct ident *p, size_t sz)
 }
 
 /*
+ * Given a list of files with the groups as set by the sender, re-assign
+ * the groups from the list of remapped group identifiers.
+ * Don't ever remap group wheel.
+ */
+void
+idents_gid_assign(struct sess *sess, struct flist *fl, size_t flsz,
+	const struct ident *gids, size_t gidsz)
+{
+	size_t	 i, j;
+
+	for (i = 0; i < flsz; i++) {
+		if (0 == fl[i].st.gid)
+			continue;
+		for (j = 0; j < gidsz; j++)
+			if ((int32_t)fl[i].st.gid == gids[j].id)
+				break;
+		assert(j < gidsz);
+		fl[i].st.gid = gids[j].mapped;
+	}
+}
+
+/*
  * Given a list of groups from the remote host, fill in our local
  * identifiers of the same names.
  * Use the remote numeric identifier if we can't find the group OR the
@@ -59,9 +81,20 @@ idents_gid_remap(struct sess *sess, struct ident *gids, size_t gidsz)
 	struct group	*grp;
 
 	for (i = 0; i < gidsz; i++) {
-		if (NULL == (grp = getgrnam(gids[i].name)))
+		assert(gids[i].id != 0);
+
+		/* 
+		 * (1) Empty names inherit.
+		 * (2) Unknown group names inherit.
+		 * (3) Group wheel inherits.
+		 * (4) Otherwise, use the local identifier.
+		 */
+
+		if (gids[i].name[0] == '\0')
 			gids[i].mapped = gids[i].id;
-		else if (0 == grp->gr_gid)
+		else if ((grp = getgrnam(gids[i].name)) == NULL)
+			gids[i].mapped = gids[i].id;
+		else if (grp->gr_gid == 0)
 			gids[i].mapped = gids[i].id;
 		else
 			gids[i].mapped = grp->gr_gid;
@@ -74,6 +107,7 @@ idents_gid_remap(struct sess *sess, struct ident *gids, size_t gidsz)
 /*
  * If "gid" is not part of the list of known groups, add it.
  * This also verifies that the group name isn't too long.
+ * Does nothing with group zero.
  * Return zero on failure, non-zero on success.
  */
 int
@@ -83,6 +117,9 @@ idents_gid_add(struct sess *sess, struct ident **gids, size_t *gidsz, gid_t gid)
 	size_t		 i, sz;
 	void		*pp;
 
+	if (gid == 0)
+		return 1;
+
 	for (i = 0; i < *gidsz; i++)
 		if ((*gids)[i].id == (int32_t)gid)
 			return 1;
@@ -94,13 +131,13 @@ idents_gid_add(struct sess *sess, struct ident **gids, size_t *gidsz, gid_t gid)
 	 */
 
 	assert(i == *gidsz);
-	if (NULL == (grp = getgrgid(gid))) {
+	if ((grp = getgrgid(gid)) == NULL) {
 		ERR(sess, "%u: unknown gid", gid);
 		return 0;
 	} else if ((sz = strlen(grp->gr_name)) > UINT8_MAX) {
 		ERRX(sess, "%u: group name too long: %s", gid, grp->gr_name);
 		return 0;
-	} else if (0 == sz) {
+	} else if (sz == 0) {
 		ERRX(sess, "%u: group name zero-length", gid);
 		return 0;
 	}
@@ -108,7 +145,7 @@ idents_gid_add(struct sess *sess, struct ident **gids, size_t *gidsz, gid_t gid)
 	/* Add the group to the array. */
 
 	pp = reallocarray(*gids, *gidsz + 1, sizeof(struct ident));
-	if (NULL == pp) {
+	if (pp == NULL) {
 		ERR(sess, "reallocarray");
 		return 0;
 	}
@@ -139,7 +176,8 @@ idents_send(struct sess *sess,
 	size_t	 i, sz;
 
 	for (i = 0; i < idsz; i++) {
-		assert(NULL != ids[i].name);
+		assert(ids[i].name != NULL);
+		assert(ids[i].id != 0);
 		sz = strlen(ids[i].name);
 		assert(sz > 0 && sz <= UINT8_MAX);
 		if (!io_write_int(sess, fd, ids[i].id)) {
@@ -180,24 +218,35 @@ idents_recv(struct sess *sess,
 		if (!io_read_int(sess, fd, &id)) {
 			ERRX1(sess, "io_read_int");
 			return 0;
-		} else if (0 == id)
+		} else if (id == 0)
 			break;
 		
 		pp = reallocarray(*ids, 
 			*idsz + 1, sizeof(struct ident));
-		if (NULL == pp) {
+		if (pp == NULL) {
 			ERR(sess, "reallocarray");
 			return 0;
 		}
 		*ids = pp;
 		memset(&(*ids)[*idsz], 0, sizeof(struct ident));
+
+		/*
+		 * When reading the size, warn if we get a group size of
+		 * zero.
+		 * The spec doesn't allow this, but we might have a
+		 * noncomformant or adversarial sender.
+		 */
+
 		if (!io_read_byte(sess, fd, &sz)) {
 			ERRX1(sess, "io_read_byte");
 			return 0;
-		}
+		} else if (0 == sz)
+			WARNX(sess, "zero-length group name "
+				"in group list");
+
 		(*ids)[*idsz].id = id;
 		(*ids)[*idsz].name = calloc(sz + 1, 1);
-		if (NULL == (*ids)[*idsz].name) {
+		if ((*ids)[*idsz].name == NULL) {
 			ERR(sess, "calloc");
 			return 0;
 		}
diff --git a/usr.bin/rsync/rsync.5 b/usr.bin/rsync/rsync.5
index 05a349f9101..e133131680d 100644
--- a/usr.bin/rsync/rsync.5
+++ b/usr.bin/rsync/rsync.5
@@ -1,4 +1,4 @@
-.\"	$OpenBSD: rsync.5,v 1.4 2019/02/12 19:11:54 benno Exp $
+.\"	$OpenBSD: rsync.5,v 1.5 2019/02/12 19:13:03 benno Exp $
 .\"
 .\" Copyright (c) 2019 Kristaps Dzonsons <kristaps@bsd.lv>
 .\"
@@ -258,15 +258,17 @@ If the status byte is zero, the file-list has terminated.
 If
 .Fl g
 has been specified, the sender sends the list of all groups encountered
-in the file list:
+in the file list.
+Group zero (commonly wheel) is never transmitted, as it has identifier
+zero and would corrupt the list.
 .Pp
 .Bl -enum -compact
 .It
 group identifier or zero to indicate end of set (integer)
 .It
-length of group (byte)
+non-zero length of group (byte)
 .It
-group name (prior length)
+non-empty group name (prior length)
 .El
 .Pp
 The sender then sends any IO error values, which for