From: claudio <claudio@openbsd.org>
Date: Mon, 17 May 2021 11:49:01 +0000 (+0000)
Subject: The openat() then fstat() pattern only works if one is sure the file being
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=e107519e0dca9b2a35b765030c89e0eaa520ff11;p=openbsd

The openat() then fstat() pattern only works if one is sure the file being
opened is a regular file. In other cases this may block in openat() (since
the O_NONBLOCK flag removed). Switch to fstatat() and then openat() to
protect from involuntary side-effects (like opening a device node) and
possible hangs.
OK benno@
---

diff --git a/usr.bin/rsync/uploader.c b/usr.bin/rsync/uploader.c
index 03db71643aa..707e85f1338 100644
--- a/usr.bin/rsync/uploader.c
+++ b/usr.bin/rsync/uploader.c
@@ -1,4 +1,4 @@
-/*	$Id: uploader.c,v 1.25 2021/05/06 07:35:22 claudio Exp $ */
+/*	$Id: uploader.c,v 1.26 2021/05/17 11:49:01 claudio Exp $ */
 /*
  * Copyright (c) 2019 Kristaps Dzonsons <kristaps@bsd.lv>
  * Copyright (c) 2019 Florian Obser <florian@openbsd.org>
@@ -79,7 +79,7 @@ log_dir(struct sess *sess, const struct flist *f)
  * operator that we're a link.
  */
 static void
-log_link(struct sess *sess, const struct flist *f)
+log_symlink(struct sess *sess, const struct flist *f)
 {
 
 	if (!sess->opts->server)
@@ -181,7 +181,7 @@ pre_link(struct upload *p, struct sess *sess)
 		return 0;
 	}
 	if (sess->opts->dry_run) {
-		log_link(sess, f);
+		log_symlink(sess, f);
 		return 0;
 	}
 
@@ -259,7 +259,7 @@ pre_link(struct upload *p, struct sess *sess)
 		free(temp);
 	}
 
-	log_link(sess, f);
+	log_symlink(sess, f);
 	return 0;
 }
 
@@ -645,6 +645,7 @@ pre_file(const struct upload *p, int *filefd, struct stat *st,
     struct sess *sess)
 {
 	const struct flist *f;
+	int rc;
 
 	f = &p->fl[p->idx];
 	assert(S_ISREG(f->st.mode));
@@ -661,51 +662,43 @@ pre_file(const struct upload *p, int *filefd, struct stat *st,
 	/*
 	 * For non dry-run cases, we'll write the acknowledgement later
 	 * in the rsync_uploader() function.
-	 * If the call to openat() fails with ENOENT, there's a
-	 * fast-path between here and the write function.
 	 */
 
-	*filefd = openat(p->rootfd, f->path,
-		O_RDONLY | O_NOFOLLOW, 0);
+	*filefd = -1;
+	rc = fstatat(p->rootfd, f->path, st, AT_SYMLINK_NOFOLLOW);
 
-	if (*filefd == -1 && errno != ENOENT) {
-		ERR("%s: openat", f->path);
+	if (rc == -1) {
+		if (errno == ENOENT)
+			return 1;
+
+		ERR("%s: fstatat", f->path);
 		return -1;
 	}
-	if (*filefd == -1)
+	if (rc != -1 && !S_ISREG(st->st_mode)) {
+		if (S_ISDIR(st->st_mode) &&
+		    unlinkat(p->rootfd, f->path, AT_REMOVEDIR) == -1) {
+			ERR("%s: unlinkat", f->path);
+			return -1;
+		}
 		return 1;
-
-	/*
-	 * Check if the file is already up to date, in which case close it
-	 * and go to the next one.
-	 */
-
-	if (fstat(*filefd, st) == -1) {
-		ERR("%s: fstat", f->path);
-		close(*filefd);
-		*filefd = -1;
-		return -1;
-	} else if (!S_ISREG(st->st_mode)) {
-		ERRX("%s: not regular", f->path);
-		close(*filefd);
-		*filefd = -1;
-		return -1;
 	}
 
 	if (st->st_size == f->st.size &&
 	    st->st_mtime == f->st.mtime) {
 		LOG3("%s: skipping: up to date", f->path);
-		if (!rsync_set_metadata(sess, 0, *filefd, f, f->path)) {
+		if (!rsync_set_metadata_at(sess, 0, p->rootfd, f, f->path)) {
 			ERRX1("rsync_set_metadata");
-			close(*filefd);
-			*filefd = -1;
 			return -1;
 		}
-		close(*filefd);
-		*filefd = -1;
 		return 0;
 	}
 
+	*filefd = openat(p->rootfd, f->path, O_RDONLY | O_NOFOLLOW, 0);
+	if (*filefd == -1 && errno != ENOENT) {
+		ERR("%s: openat", f->path);
+		return -1;
+	}
+
 	/* file needs attention */
 	return 1;
 }
@@ -785,8 +778,7 @@ rsync_uploader(struct upload *u, int *fileinfd,
 	off_t		    offs;
 	int		    c;
 
-	/* This should never get called. */
-
+	/* Once finished this should never get called again. */
 	assert(u->state != UPLOAD_FINISHED);
 
 	/*