[Toybox] [PATCH] cp: fix -D (--parents)

[Jarno Mäkipää]

Alright you are right I should do better job cleaning up. I attached
reworked patch that deals with leaking issues, should apply to current
master.

So original problem with current cp.c code was: data under *src should
be kept intact since its used later in rename and friends, dirname()
is not actually needed to be called since we dont need modify src with
flag(D). getbasename() is toybox/lib/ version of basename() but it
shares same trait as basename that input data may be modified so to be
save side strdup before and free strdup after getbasename result has
been used.

things that allocate here
s = fileunderdir()
s = strdup()
TT.destname = xmprintf()

TT.destname have free() near end of for loop. (And to be completely
clean extra free() could be put into fileunderdir() error_msg case
too. But I left it out since I dont know when this case even happens?)

I did not free() my strdup before but if we move free(s) from
fileunderdir check code block few lines out of else block so it covers
both if and else branches we get both cases freed.

Now changes look like this and valgrind shows that we are leaking 2
blocks instead of 3, but I have feeling they are not in this section
of cp.c since normal file to file case leaks 2 blocks also.

     if (destdir) {
-      char *s = FLAG(D) ? dirname(src) : getbasename(src);
-
-      TT.destname = xmprintf("%s/%s", destname, s);
+      char *s;
       if (FLAG(D)) {
+        TT.destname = xmprintf("%s/%s", destname, src);
         if (!(s = fileunderdir(TT.destname, destname))) {
           error_msg("%s not under %s", TT.destname, destname);
           continue;
         }
         // TODO: .. follows abspath, not links...
-        free(s);
         mkpath(TT.destname);
+      } else {
+        s = strdup(src);
+        TT.destname = xmprintf("%s/%s", destname, getbasename(s));
       }
+      free(s);
     } else TT.destname = destname;

On Sat, Mar 7, 2020 at 2:14 AM Rob Landley <rob at landley.net> wrote:
>
> On 3/4/20 12:51 PM, Jarno Mäkipää wrote:
> > I know you are busy and backlog grows, but friendly reminder that this
> > patch is still here.
>
> Sorry.
>
> You add two allocations (strdup and xmprintf()) but no corresponding free. It's
> not in cp_node(), so it's not leaking per file copied, but it _is_ leaking per
> file listed on the command line, which is probably ok but I'm less comfortable
> with it these days than I used to be.
>
> Mostly, I want to get it clear in my head why it needs to copy it _twice_, so I
> have a window open for it, but my head is full of shell corner cases right now
> and I'm making "melt through this glacier to the other side" style progress.
>
> Rob
>
> P.S. There was a Doctor Who episode about making that kind of progress a couple
> years back, and if you google for "doctor who hugo" and then click on the 2016
> entry, Google pulls up a page with the exact same summary information at the top
> that does an endless loop as you click on the same entry over and over. Which I
> suppose is apropos for the episode.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-cp-fix-D-parents-REWORK.patch
Type: text/x-patch
Size: 2112 bytes
Desc: not available
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20200308/4162e5b0/attachment-0003.bin>