Claudio Jeker([email protected]) on 2021.05.07 17:12:16 +0200:
> So yesterday I committed a change to simplify file handling. This removed
> the O_NONBLOCK flag from openat() but today I realized that this was a bit
> premature. The code at that point does not know if the file is actually a
> regular file and so if you put a fifo in place of a regular file it will
> hang rsync.
right :/
>
> Anyway doing a blind open of any file here is far from ideal. There may be
> involuntary side-effects especially on device nodes. So better change the
> code to do the fstatat() first and only open the file if it is indeed a
> regular file.
>
> While there change log_link to log_symlink which is more precise and also
> fix a comment that missed some context.
>
> OK?
yes.
> --
> :wq Claudio
>
> Index: uploader.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/rsync/uploader.c,v
> retrieving revision 1.25
> diff -u -p -r1.25 uploader.c
> --- uploader.c 6 May 2021 07:35:22 -0000 1.25
> +++ uploader.c 7 May 2021 15:03:03 -0000
> @@ -79,7 +79,7 @@ log_dir(struct sess *sess, const struct
> * 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 *
> 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 *
> free(temp);
> }
>
> - log_link(sess, f);
> + log_symlink(sess, f);
> return 0;
> }
>
> @@ -645,6 +645,7 @@ pre_file(const struct upload *p, int *fi
> 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 *fi
> /*
> * 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 *fi
> off_t offs;
> int c;
>
> - /* This should never get called. */
> -
> + /* Once finished this should never get called again. */
> assert(u->state != UPLOAD_FINISHED);
>
> /*
>
