[patch] rsync: fix free() on uninitialized pointer with -rx and same device

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

[patch] rsync: fix free() on uninitialized pointer with -rx and same device

Hiltjo Posthuma
Hi,

I noticed a free() issue on an uninitialized pointer on a certain condition.

To reproduce:

        mkdir -p /tmp/test /tmp/plop
        openrsync -rx /tmp/test/ /tmp/plop/

Result:

        openrsync(3470) in free(): bogus pointer (double free?) 0x7f7ffffdcdc8
        Abort trap (core dumped)

The check does not match the condition checked before free(xdev);

        if (sess->opts->one_file_system &&
            ent->fts_statp->st_dev != st.st_dev) {


The patch below fixes it and simplifies the logic:


diff --git usr.bin/rsync/flist.c usr.bin/rsync/flist.c
index e1f41b1a108..1b3f9e40f62 100644
--- usr.bin/rsync/flist.c
+++ usr.bin/rsync/flist.c
@@ -808,7 +808,7 @@ flist_gen_dirent(struct sess *sess, char *root, struct flist **fl, size_t *sz,
  FTSENT *ent;
  struct flist *f;
  size_t flsz = 0, stripdir;
- dev_t *xdev;
+ dev_t *xdev = NULL;
  struct stat st;
 
  cargv[0] = root;
@@ -1008,8 +1008,7 @@ flist_gen_dirent(struct sess *sess, char *root, struct flist **fl, size_t *sz,
  rc = 1;
 out:
  fts_close(fts);
- if (sess->opts->one_file_system)
- free(xdev);
+ free(xdev);
  return rc;
 }
 

--
Kind regards,
Hiltjo

Reply | Threaded
Open this post in threaded view
|

Re: [patch] rsync: fix free() on uninitialized pointer with -rx and same device

Christian Weisgerber
Hiltjo Posthuma:

> I noticed a free() issue on an uninitialized pointer on a certain condition.
>
> To reproduce:
>
> mkdir -p /tmp/test /tmp/plop
> openrsync -rx /tmp/test/ /tmp/plop/
>
> Result:
>
> openrsync(3470) in free(): bogus pointer (double free?) 0x7f7ffffdcdc8
> Abort trap (core dumped)
>
> The patch below fixes it and simplifies the logic:

I agree.  However, the (re)allocation of xdev also looks bogus.

How about this?

(Also, the realloc idiom is exactly the one the man page warns
against.  Do we care here?)

Index: flist.c
===================================================================
RCS file: /cvs/src/usr.bin/rsync/flist.c,v
retrieving revision 1.27
diff -u -p -r1.27 flist.c
--- flist.c 2 Jun 2019 14:29:58 -0000 1.27
+++ flist.c 5 Jun 2019 22:15:10 -0000
@@ -808,7 +808,7 @@ flist_gen_dirent(struct sess *sess, char
  FTSENT *ent;
  struct flist *f;
  size_t flsz = 0, stripdir;
- dev_t *xdev;
+ dev_t *xdev = NULL;
  struct stat st;
 
  cargv[0] = root;
@@ -931,7 +931,8 @@ flist_gen_dirent(struct sess *sess, char
     !S_ISDIR(ent->fts_statp->st_mode))
  continue;
 
- if ((xdev = malloc(sizeof(dev_t))) == NULL) {
+ if (xdev == NULL &&
+    (xdev = malloc(sizeof(dev_t))) == NULL) {
  ERRX1("malloc");
  goto out;
  }
@@ -946,8 +947,8 @@ flist_gen_dirent(struct sess *sess, char
  continue;
 
  if (nxdev)
- if ((xdev = realloc(xdev, sizeof(dev_t))) ==
-    NULL) {
+ if ((xdev = reallocarray(xdev, nxdev + 1,
+    sizeof(dev_t))) == NULL) {
  ERRX1("realloc");
  goto out;
  }
@@ -1008,8 +1009,7 @@ flist_gen_dirent(struct sess *sess, char
  rc = 1;
 out:
  fts_close(fts);
- if (sess->opts->one_file_system)
- free(xdev);
+ free(xdev);
  return rc;
 }
 
--
Christian "naddy" Weisgerber                          [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [patch] rsync: fix free() on uninitialized pointer with -rx and same device

Christian Weisgerber
Christian Weisgerber:

> > To reproduce:
> >
> > mkdir -p /tmp/test /tmp/plop
> > openrsync -rx /tmp/test/ /tmp/plop/
> >
> > Result:
> >
> > openrsync(3470) in free(): bogus pointer (double free?) 0x7f7ffffdcdc8
> > Abort trap (core dumped)
> >
> > The patch below fixes it and simplifies the logic:
>
> I agree.  However, the (re)allocation of xdev also looks bogus.
>
> How about this?
>
> (Also, the realloc idiom is exactly the one the man page warns
> against.  Do we care here?)

Well, let's not set a bad example.

Index: flist.c
===================================================================
RCS file: /cvs/src/usr.bin/rsync/flist.c,v
retrieving revision 1.27
diff -u -p -r1.27 flist.c
--- flist.c 2 Jun 2019 14:29:58 -0000 1.27
+++ flist.c 5 Jun 2019 22:36:28 -0000
@@ -808,7 +808,7 @@ flist_gen_dirent(struct sess *sess, char
  FTSENT *ent;
  struct flist *f;
  size_t flsz = 0, stripdir;
- dev_t *xdev;
+ dev_t *newxdev, *xdev = NULL;
  struct stat st;
 
  cargv[0] = root;
@@ -931,7 +931,8 @@ flist_gen_dirent(struct sess *sess, char
     !S_ISDIR(ent->fts_statp->st_mode))
  continue;
 
- if ((xdev = malloc(sizeof(dev_t))) == NULL) {
+ if (xdev == NULL &&
+    (xdev = malloc(sizeof(dev_t))) == NULL) {
  ERRX1("malloc");
  goto out;
  }
@@ -945,12 +946,14 @@ flist_gen_dirent(struct sess *sess, char
  if (flag)
  continue;
 
- if (nxdev)
- if ((xdev = realloc(xdev, sizeof(dev_t))) ==
-    NULL) {
+ if (nxdev) {
+ if ((newxdev = reallocarray(xdev, nxdev + 1,
+    sizeof(dev_t))) == NULL) {
  ERRX1("realloc");
  goto out;
  }
+ xdev = newxdev;
+ }
  xdev[nxdev] = ent->fts_statp->st_dev;
  nxdev++;
  }
@@ -1008,8 +1011,7 @@ flist_gen_dirent(struct sess *sess, char
  rc = 1;
 out:
  fts_close(fts);
- if (sess->opts->one_file_system)
- free(xdev);
+ free(xdev);
  return rc;
 }
 
--
Christian "naddy" Weisgerber                          [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [patch] rsync: fix free() on uninitialized pointer with -rx and same device

Hiltjo Posthuma
On Thu, Jun 06, 2019 at 12:37:13AM +0200, Christian Weisgerber wrote:

> Christian Weisgerber:
>
> > > To reproduce:
> > >
> > > mkdir -p /tmp/test /tmp/plop
> > > openrsync -rx /tmp/test/ /tmp/plop/
> > >
> > > Result:
> > >
> > > openrsync(3470) in free(): bogus pointer (double free?) 0x7f7ffffdcdc8
> > > Abort trap (core dumped)
> > >
> > > The patch below fixes it and simplifies the logic:
> >
> > I agree.  However, the (re)allocation of xdev also looks bogus.
> >

Hi,

I noticed the realloc issue also and wanted to send a separate patch later for
it. Nice it is addressed also :)

> > How about this?
> >
> > (Also, the realloc idiom is exactly the one the man page warns
> > against.  Do we care here?)
>
> Well, let's not set a bad example.
>
> Index: flist.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/rsync/flist.c,v
> retrieving revision 1.27
> diff -u -p -r1.27 flist.c
> --- flist.c 2 Jun 2019 14:29:58 -0000 1.27
> +++ flist.c 5 Jun 2019 22:36:28 -0000
> @@ -808,7 +808,7 @@ flist_gen_dirent(struct sess *sess, char
>   FTSENT *ent;
>   struct flist *f;
>   size_t flsz = 0, stripdir;
> - dev_t *xdev;
> + dev_t *newxdev, *xdev = NULL;
>   struct stat st;
>  
>   cargv[0] = root;
> @@ -931,7 +931,8 @@ flist_gen_dirent(struct sess *sess, char
>      !S_ISDIR(ent->fts_statp->st_mode))
>   continue;
>  
> - if ((xdev = malloc(sizeof(dev_t))) == NULL) {
> + if (xdev == NULL &&
> +    (xdev = malloc(sizeof(dev_t))) == NULL) {
>   ERRX1("malloc");
>   goto out;
>   }
> @@ -945,12 +946,14 @@ flist_gen_dirent(struct sess *sess, char
>   if (flag)
>   continue;
>  
> - if (nxdev)
> - if ((xdev = realloc(xdev, sizeof(dev_t))) ==
> -    NULL) {
> + if (nxdev) {
> + if ((newxdev = reallocarray(xdev, nxdev + 1,
> +    sizeof(dev_t))) == NULL) {

Also could the first malloc and realloc maybe get simplified to
reallocarray(NULL, ...) ?

>   ERRX1("realloc");
>   goto out;
>   }
> + xdev = newxdev;
> + }
>   xdev[nxdev] = ent->fts_statp->st_dev;
>   nxdev++;
>   }
> @@ -1008,8 +1011,7 @@ flist_gen_dirent(struct sess *sess, char
>   rc = 1;
>  out:
>   fts_close(fts);
> - if (sess->opts->one_file_system)
> - free(xdev);
> + free(xdev);
>   return rc;
>  }
>  
> --
> Christian "naddy" Weisgerber                          [hidden email]
>

--
Kind regards,
Hiltjo

Reply | Threaded
Open this post in threaded view
|

Re: [patch] rsync: fix free() on uninitialized pointer with -rx and same device

Florian Obser-2
In reply to this post by Christian Weisgerber
OK

On Thu, Jun 06, 2019 at 12:37:13AM +0200, Christian Weisgerber wrote:

> Christian Weisgerber:
>
> > > To reproduce:
> > >
> > > mkdir -p /tmp/test /tmp/plop
> > > openrsync -rx /tmp/test/ /tmp/plop/
> > >
> > > Result:
> > >
> > > openrsync(3470) in free(): bogus pointer (double free?) 0x7f7ffffdcdc8
> > > Abort trap (core dumped)
> > >
> > > The patch below fixes it and simplifies the logic:
> >
> > I agree.  However, the (re)allocation of xdev also looks bogus.
> >
> > How about this?
> >
> > (Also, the realloc idiom is exactly the one the man page warns
> > against.  Do we care here?)
>
> Well, let's not set a bad example.
>
> Index: flist.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/rsync/flist.c,v
> retrieving revision 1.27
> diff -u -p -r1.27 flist.c
> --- flist.c 2 Jun 2019 14:29:58 -0000 1.27
> +++ flist.c 5 Jun 2019 22:36:28 -0000
> @@ -808,7 +808,7 @@ flist_gen_dirent(struct sess *sess, char
>   FTSENT *ent;
>   struct flist *f;
>   size_t flsz = 0, stripdir;
> - dev_t *xdev;
> + dev_t *newxdev, *xdev = NULL;
>   struct stat st;
>  
>   cargv[0] = root;
> @@ -931,7 +931,8 @@ flist_gen_dirent(struct sess *sess, char
>      !S_ISDIR(ent->fts_statp->st_mode))
>   continue;
>  
> - if ((xdev = malloc(sizeof(dev_t))) == NULL) {
> + if (xdev == NULL &&
> +    (xdev = malloc(sizeof(dev_t))) == NULL) {
>   ERRX1("malloc");
>   goto out;
>   }
> @@ -945,12 +946,14 @@ flist_gen_dirent(struct sess *sess, char
>   if (flag)
>   continue;
>  
> - if (nxdev)
> - if ((xdev = realloc(xdev, sizeof(dev_t))) ==
> -    NULL) {
> + if (nxdev) {
> + if ((newxdev = reallocarray(xdev, nxdev + 1,
> +    sizeof(dev_t))) == NULL) {
>   ERRX1("realloc");
>   goto out;
>   }
> + xdev = newxdev;
> + }
>   xdev[nxdev] = ent->fts_statp->st_dev;
>   nxdev++;
>   }
> @@ -1008,8 +1011,7 @@ flist_gen_dirent(struct sess *sess, char
>   rc = 1;
>  out:
>   fts_close(fts);
> - if (sess->opts->one_file_system)
> - free(xdev);
> + free(xdev);
>   return rc;
>  }
>  
> --
> Christian "naddy" Weisgerber                          [hidden email]
>

--
I'm not entirely sure you are real.

Reply | Threaded
Open this post in threaded view
|

Re: [patch] rsync: fix free() on uninitialized pointer with -rx and same device

Björn Ketelaars
In reply to this post by Christian Weisgerber
On Thu 06/06/2019 00:37, Christian Weisgerber wrote:

> Christian Weisgerber:
>
> > > To reproduce:
> > >
> > > mkdir -p /tmp/test /tmp/plop
> > > openrsync -rx /tmp/test/ /tmp/plop/
> > >
> > > Result:
> > >
> > > openrsync(3470) in free(): bogus pointer (double free?) 0x7f7ffffdcdc8
> > > Abort trap (core dumped)
> > >
> > > The patch below fixes it and simplifies the logic:
> >
> > I agree.  However, the (re)allocation of xdev also looks bogus.
> >
> > How about this?
> >
> > (Also, the realloc idiom is exactly the one the man page warns
> > against.  Do we care here?)
>
> Well, let's not set a bad example.
>

Guess I'm late to the party...I only read Hiltjo's mail, and the
replies, after being contacted offlist by naddy@.

Diff below is based on the latest diff from naddy@. Changes:
- reallocarray likes type_t, as such changes type of nxdev and i;
- use reallocarray instead of malloc as xdev is initialised as NULL.

diff --git flist.c flist.c
index e1f41b1a108..4c552ce3c5c 100644
--- flist.c
+++ flist.c
@@ -803,12 +803,12 @@ flist_gen_dirent(struct sess *sess, char *root, struct flist **fl, size_t *sz,
     size_t *max)
 {
  char *cargv[2], *cp;
- int rc = 0, nxdev = 0, flag, i;
+ int rc = 0, flag;
  FTS *fts;
  FTSENT *ent;
  struct flist *f;
- size_t flsz = 0, stripdir;
- dev_t *xdev;
+ size_t i, flsz = 0, nxdev = 0, stripdir;
+ dev_t *newxdev, *xdev = NULL;
  struct stat st;
 
  cargv[0] = root;
@@ -931,11 +931,6 @@ flist_gen_dirent(struct sess *sess, char *root, struct flist **fl, size_t *sz,
     !S_ISDIR(ent->fts_statp->st_mode))
  continue;
 
- if ((xdev = malloc(sizeof(dev_t))) == NULL) {
- ERRX1("malloc");
- goto out;
- }
-
  flag = 0;
  for (i = 0; i < nxdev; i++)
  if (xdev[i] == ent->fts_statp->st_dev) {
@@ -945,12 +940,12 @@ flist_gen_dirent(struct sess *sess, char *root, struct flist **fl, size_t *sz,
  if (flag)
  continue;
 
- if (nxdev)
- if ((xdev = realloc(xdev, sizeof(dev_t))) ==
-    NULL) {
- ERRX1("realloc");
- goto out;
- }
+ if ((newxdev = reallocarray(xdev, nxdev + 1,
+    sizeof(dev_t))) == NULL) {
+ ERRX1("reallocarray");
+ goto out;
+ }
+ xdev = newxdev;
  xdev[nxdev] = ent->fts_statp->st_dev;
  nxdev++;
  }
@@ -1008,8 +1003,7 @@ flist_gen_dirent(struct sess *sess, char *root, struct flist **fl, size_t *sz,
  rc = 1;
 out:
  fts_close(fts);
- if (sess->opts->one_file_system)
- free(xdev);
+ free(xdev);
  return rc;
 }
 

Reply | Threaded
Open this post in threaded view
|

Re: [patch] rsync: fix free() on uninitialized pointer with -rx and same device

Christian Weisgerber
Björn Ketelaars:

> Diff below is based on the latest diff from naddy@. Changes:
> - reallocarray likes type_t, as such changes type of nxdev and i;
> - use reallocarray instead of malloc as xdev is initialised as NULL.

ok naddy@

--
Christian "naddy" Weisgerber                          [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [patch] rsync: fix free() on uninitialized pointer with -rx and same device

Hiltjo Posthuma
On Thu, Jun 06, 2019 at 02:14:05PM +0200, Christian Weisgerber wrote:

> Björn Ketelaars:
>
> > Diff below is based on the latest diff from naddy@. Changes:
> > - reallocarray likes type_t, as such changes type of nxdev and i;
> > - use reallocarray instead of malloc as xdev is initialised as NULL.
>
> ok naddy@
>
> --
> Christian "naddy" Weisgerber                          [hidden email]
>

Looks good to me.

I have 2 or 3 more patches locally. It would be nice if the above patch can get
committed so I can rebase and send a clean patch.

Thanks,

--
Kind regards,
Hiltjo

Reply | Threaded
Open this post in threaded view
|

Re: [patch] rsync: fix free() on uninitialized pointer with -rx and same device

Hiltjo Posthuma
On Wed, Jun 12, 2019 at 06:58:31PM +0200, Hiltjo Posthuma wrote:

> On Thu, Jun 06, 2019 at 02:14:05PM +0200, Christian Weisgerber wrote:
> > Björn Ketelaars:
> >
> > > Diff below is based on the latest diff from naddy@. Changes:
> > > - reallocarray likes type_t, as such changes type of nxdev and i;
> > > - use reallocarray instead of malloc as xdev is initialised as NULL.
> >
> > ok naddy@
> >
> > --
> > Christian "naddy" Weisgerber                          [hidden email]
> >
>
> Looks good to me.
>
> I have 2 or 3 more patches locally. It would be nice if the above patch can get
> committed so I can rebase and send a clean patch.
>

Hi,

This patch has not been committed yet. Is it possible to commit it or does it
need further work/review? If so, I'm happy to help.

--
Kind regards,
Hiltjo

Reply | Threaded
Open this post in threaded view
|

Re: [patch] rsync: fix free() on uninitialized pointer with -rx and same device

Sebastian Benoit-3
Hiltjo Posthuma([hidden email]) on 2019.06.21 13:32:10 +0200:

> On Wed, Jun 12, 2019 at 06:58:31PM +0200, Hiltjo Posthuma wrote:
> > On Thu, Jun 06, 2019 at 02:14:05PM +0200, Christian Weisgerber wrote:
> > > Bj??rn Ketelaars:
> > >
> > > > Diff below is based on the latest diff from naddy@. Changes:
> > > > - reallocarray likes type_t, as such changes type of nxdev and i;
> > > > - use reallocarray instead of malloc as xdev is initialised as NULL.
> > >
> > > ok naddy@
> > >
> > > --
> > > Christian "naddy" Weisgerber                          [hidden email]
> > >
> >
> > Looks good to me.
> >
> > I have 2 or 3 more patches locally. It would be nice if the above patch can get
> > committed so I can rebase and send a clean patch.
> >
>
> Hi,
>
> This patch has not been committed yet. Is it possible to commit it or does it
> need further work/review? If so, I'm happy to help.

I just commited it,
Thanks forthe reminder!

/Benno