[patch] openrsync blk_flush() fd leak in blk_match()

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

[patch] openrsync blk_flush() fd leak in blk_match()

Hiltjo Posthuma
Hi,

I think the fd can leak when blk_flush() fails in blocks.c blk_match(), because
ERR and ERRX1 does not to terminate. Am I correct?

I also (hopefully) simplified the logic a bit (set nfd = -1 and goto out).

Thanks for working on (open)rsync!

Patch below:


diff --git a/usr.bin/rsync/blocks.c b/usr.bin/rsync/blocks.c
index 6f1d8d2fb6f..984e048a730 100644
--- a/usr.bin/rsync/blocks.c
+++ b/usr.bin/rsync/blocks.c
@@ -238,7 +238,7 @@ int
 blk_match(struct sess *sess, int fd,
  const struct blkset *blks, const char *path)
 {
- int nfd, rc = 0, c;
+ int nfd = -1, rc = 0, c;
  struct stat st;
  void *map = MAP_FAILED;
  size_t mapsz;
@@ -248,11 +248,10 @@ blk_match(struct sess *sess, int fd,
 
  if (-1 == (nfd = open(path, O_RDONLY, 0))) {
  ERR(sess, "%s: open", path);
- return 0;
+ goto out;
  } else if (-1 == fstat(nfd, &st)) {
  ERR(sess, "%s: fstat", path);
- close(nfd);
- return 0;
+ goto out;
  }
 
  /*
@@ -264,8 +263,7 @@ blk_match(struct sess *sess, int fd,
  map = mmap(NULL, mapsz, PROT_READ, MAP_SHARED, nfd, 0);
  if (MAP_FAILED == map) {
  ERR(sess, "%s: mmap", path);
- close(nfd);
- return 0;
+ goto out;
  }
  }
 
@@ -287,7 +285,7 @@ blk_match(struct sess *sess, int fd,
  } else {
  if ( ! blk_flush(sess, fd, map, st.st_size, 0)) {
  ERRX1(sess, "blk_flush");
- return 0;
+ goto out;
  }
  LOG3(sess, "%s: flushed (un-chunked) %jd B, 100%% "
  "upload ratio", path, (intmax_t)st.st_size);
@@ -310,7 +308,8 @@ blk_match(struct sess *sess, int fd,
 out:
  if (MAP_FAILED != map)
  munmap(map, mapsz);
- close(nfd);
+ if (-1 != nfd)
+ close(nfd);
  return rc;
 }
 

--
Kind regards,
Hiltjo

Reply | Threaded
Open this post in threaded view
|

Re: [patch] openrsync blk_flush() fd leak in blk_match()

Sebastian Benoit-3
commited, thanks!

Hiltjo Posthuma([hidden email]) on 2019.02.11 20:55:09 +0100:

> Hi,
>
> I think the fd can leak when blk_flush() fails in blocks.c blk_match(), because
> ERR and ERRX1 does not to terminate. Am I correct?
>
> I also (hopefully) simplified the logic a bit (set nfd = -1 and goto out).
>
> Thanks for working on (open)rsync!
>
> Patch below:
>
>
> diff --git a/usr.bin/rsync/blocks.c b/usr.bin/rsync/blocks.c
> index 6f1d8d2fb6f..984e048a730 100644
> --- a/usr.bin/rsync/blocks.c
> +++ b/usr.bin/rsync/blocks.c
> @@ -238,7 +238,7 @@ int
>  blk_match(struct sess *sess, int fd,
>   const struct blkset *blks, const char *path)
>  {
> - int nfd, rc = 0, c;
> + int nfd = -1, rc = 0, c;
>   struct stat st;
>   void *map = MAP_FAILED;
>   size_t mapsz;
> @@ -248,11 +248,10 @@ blk_match(struct sess *sess, int fd,
>  
>   if (-1 == (nfd = open(path, O_RDONLY, 0))) {
>   ERR(sess, "%s: open", path);
> - return 0;
> + goto out;
>   } else if (-1 == fstat(nfd, &st)) {
>   ERR(sess, "%s: fstat", path);
> - close(nfd);
> - return 0;
> + goto out;
>   }
>  
>   /*
> @@ -264,8 +263,7 @@ blk_match(struct sess *sess, int fd,
>   map = mmap(NULL, mapsz, PROT_READ, MAP_SHARED, nfd, 0);
>   if (MAP_FAILED == map) {
>   ERR(sess, "%s: mmap", path);
> - close(nfd);
> - return 0;
> + goto out;
>   }
>   }
>  
> @@ -287,7 +285,7 @@ blk_match(struct sess *sess, int fd,
>   } else {
>   if ( ! blk_flush(sess, fd, map, st.st_size, 0)) {
>   ERRX1(sess, "blk_flush");
> - return 0;
> + goto out;
>   }
>   LOG3(sess, "%s: flushed (un-chunked) %jd B, 100%% "
>   "upload ratio", path, (intmax_t)st.st_size);
> @@ -310,7 +308,8 @@ blk_match(struct sess *sess, int fd,
>  out:
>   if (MAP_FAILED != map)
>   munmap(map, mapsz);
> - close(nfd);
> + if (-1 != nfd)
> + close(nfd);
>   return rc;
>  }
>  
>
> --
> Kind regards,
> Hiltjo
>