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

[Jarno Mäkipää]

Oh nevermind half of what i just said....

getbasename() does not change data. idk what I was looking at on my
ipad when I wrote half of the msg. Well I had few versions of old
toybox getdirname() and some random c library basename implementations
on browser tabs open so mix up is just human error...

strdup on else case is not needed, and therefore fix can be just
simplified by replacing dirname only, so the minimal fix is just


-      char *s = FLAG(D) ? dirname(src) : getbasename(src);
+      char *s = FLAG(D) ? src : getbasename(src);

this is probably something you liked to see... :)


-Jarno

ps. I still dont understand how fileunderdir() if is triggered....

On Sun, Mar 8, 2020 at 5:14 PM Jarno Mäkipää <jmakip87 at gmail.com> wrote:
>
> 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-MINIMAL-FIX.patch
Type: text/x-patch
Size: 1504 bytes
Desc: not available
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20200308/2410b12e/attachment-0003.bin>