[patch] rsync: fix another double close socket descriptor

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

[patch] rsync: fix another double close socket descriptor

Hiltjo Posthuma
Hi,

I noticed when using openrsync with a remote and a ssh_prog set (-e option) the
socket is closed twice also.

Reproducable with:

ktrace openrsync -e echo -av rsync://127.0.0.1/ftp/ a

kdump:

  70262 openrsync CALL  close(3)
  70262 openrsync RET   close 0
> 70262 openrsync CALL  close(3)
> 70262 openrsync RET   close -1 errno 9 Bad file descriptor
  70262 openrsync CALL  kbind(0x7f7ffffeb390,24,0x26f671b79e307e66)
  70262 openrsync RET   kbind 0
  70262 openrsync CALL  wait4(15180,0x7f7ffffeb468,0<>,0)
  70262 openrsync RET   wait4 15180/0x3b4c


The patch below fixes this, but has a behaviour change: when the return value
rc = 0 the socket is not closed anymore directly.

I included 2 tiny fixes:
- In blocks.c have_md was initialized to zero twice.
- flist.c has a small typo in a comment.


diff --git usr.bin/rsync/blocks.c usr.bin/rsync/blocks.c
index f6a3575caab..09f09b44e0e 100644
--- usr.bin/rsync/blocks.c
+++ usr.bin/rsync/blocks.c
@@ -55,7 +55,6 @@ blk_find(struct sess *sess, const void *buf, off_t size, off_t offs,
  assert(remain);
  osz = remain < (off_t)blks->len ? remain : (off_t)blks->len;
  fhash = hash_fast(buf + offs, (size_t)osz);
- have_md = 0;
 
  /*
  * Start with our match hint.
diff --git usr.bin/rsync/flist.c usr.bin/rsync/flist.c
index f8a6f00a149..447907735af 100644
--- usr.bin/rsync/flist.c
+++ usr.bin/rsync/flist.c
@@ -53,7 +53,7 @@
 #define FLIST_TIME_SAME  0x0080 /* time is repeat */
 
 /*
- * Requied way to sort a filename list.
+ * Required way to sort a filename list.
  */
 static int
 flist_cmp(const void *p1, const void *p2)
diff --git usr.bin/rsync/socket.c usr.bin/rsync/socket.c
index 1a0844f1313..b2cc6b6ac0e 100644
--- usr.bin/rsync/socket.c
+++ usr.bin/rsync/socket.c
@@ -455,6 +455,5 @@ rsync_socket(const struct opts *opts, int sd, const struct fargs *f)
  rc = 0;
 out:
  free(args);
- close(sd);
  return rc;
 }

--
Kind regards,
Hiltjo

Reply | Threaded
Open this post in threaded view
|

Re: [patch] rsync: fix another double close socket descriptor

Christian Weisgerber
On 2019-06-02, Hiltjo Posthuma <[hidden email]> wrote:

> I noticed when using openrsync with a remote and a ssh_prog set (-e option) the
> socket is closed twice also.

Yes, we need to decide whether rsync_socket() should close the
socket itself or not.  Since rsync_socket() doesn't open the socket,
I guess it shouldn't close it either.  Matches rsync_client().

ok?

Index: main.c
===================================================================
RCS file: /cvs/src/usr.bin/rsync/main.c,v
retrieving revision 1.46
diff -u -p -r1.46 main.c
--- main.c 28 May 2019 18:20:30 -0000 1.46
+++ main.c 3 Jun 2019 14:30:06 -0000
@@ -428,8 +428,10 @@ main(int argc, char *argv[])
 
  if (fargs->remote && opts.ssh_prog == NULL) {
  assert(fargs->mode == FARGS_RECEIVER);
- if ((rc = rsync_connect(&opts, &sd, fargs)) == 0)
+ if ((rc = rsync_connect(&opts, &sd, fargs)) == 0) {
  rc = rsync_socket(&opts, sd, fargs);
+ close(sd);
+ }
  exit(rc);
  }
 
@@ -484,14 +486,7 @@ main(int argc, char *argv[])
  break;
  }
 
- /*
- * If the client has an error and exits, the server may be
- * sitting around waiting to get data while we waitpid().
- * So close the connection here so that they don't hang.
- */
-
- if (rc)
- close(fds[0]);
+ close(fds[0]);
 
  if (waitpid(child, &st, 0) == -1)
  err(1, "waitpid");
Index: socket.c
===================================================================
RCS file: /cvs/src/usr.bin/rsync/socket.c,v
retrieving revision 1.24
diff -u -p -r1.24 socket.c
--- socket.c 8 May 2019 21:30:11 -0000 1.24
+++ socket.c 3 Jun 2019 14:26:44 -0000
@@ -455,6 +455,5 @@ rsync_socket(const struct opts *opts, in
  rc = 0;
 out:
  free(args);
- close(sd);
  return rc;
 }
--
Christian "naddy" Weisgerber                          [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [patch] rsync: fix another double close socket descriptor

Sebastian Benoit-3
Christian Weisgerber([hidden email]) on 2019.06.03 14:39:14 -0000:

> On 2019-06-02, Hiltjo Posthuma <[hidden email]> wrote:
>
> > I noticed when using openrsync with a remote and a ssh_prog set (-e option) the
> > socket is closed twice also.
>
> Yes, we need to decide whether rsync_socket() should close the
> socket itself or not.  Since rsync_socket() doesn't open the socket,
> I guess it shouldn't close it either.  Matches rsync_client().
>
> ok?

ok


>
> Index: main.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/rsync/main.c,v
> retrieving revision 1.46
> diff -u -p -r1.46 main.c
> --- main.c 28 May 2019 18:20:30 -0000 1.46
> +++ main.c 3 Jun 2019 14:30:06 -0000
> @@ -428,8 +428,10 @@ main(int argc, char *argv[])
>  
>   if (fargs->remote && opts.ssh_prog == NULL) {
>   assert(fargs->mode == FARGS_RECEIVER);
> - if ((rc = rsync_connect(&opts, &sd, fargs)) == 0)
> + if ((rc = rsync_connect(&opts, &sd, fargs)) == 0) {
>   rc = rsync_socket(&opts, sd, fargs);
> + close(sd);
> + }
>   exit(rc);
>   }
>  
> @@ -484,14 +486,7 @@ main(int argc, char *argv[])
>   break;
>   }
>  
> - /*
> - * If the client has an error and exits, the server may be
> - * sitting around waiting to get data while we waitpid().
> - * So close the connection here so that they don't hang.
> - */
> -
> - if (rc)
> - close(fds[0]);
> + close(fds[0]);
>  
>   if (waitpid(child, &st, 0) == -1)
>   err(1, "waitpid");
> Index: socket.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/rsync/socket.c,v
> retrieving revision 1.24
> diff -u -p -r1.24 socket.c
> --- socket.c 8 May 2019 21:30:11 -0000 1.24
> +++ socket.c 3 Jun 2019 14:26:44 -0000
> @@ -455,6 +455,5 @@ rsync_socket(const struct opts *opts, in
>   rc = 0;
>  out:
>   free(args);
> - close(sd);
>   return rc;
>  }
> --
> Christian "naddy" Weisgerber                          [hidden email]
>