Claudio Jeker(cje...@diehard.n-r-g.com) 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); > > /* >