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

[Björn Ketelaars]

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;
 }