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ää <jmaki...@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 <r...@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.
From 516d8b518635a4d506705895ff805105c1979021 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jarno=20M=C3=A4kip=C3=A4=C3=A4?= <jmaki...@gmail.com>
Date: Sat, 15 Feb 2020 13:31:48 +0200
Subject: [PATCH] cp: fix -D (--parents) (REWORK MINIMAL FIX)
add: test for -D
fix: b/c/d/FILE not copying into a/ with -D option
dirname() is not needed when handling FLAG(D) since filename under
src or dest should not be changed.
github.com/landley/toybox/issues/165
---
tests/cp.test | 7 +++++++
toys/posix/cp.c | 2 +-
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/tests/cp.test b/tests/cp.test
index dfb80ea1..5c4a7474 100755
--- a/tests/cp.test
+++ b/tests/cp.test
@@ -120,6 +120,13 @@ testing "-T file" "cp -T b file && cat file" "b\n" "" ""
testing "-T dir" "cp -T b dir 2>/dev/null || echo expected" "expected\n" "" ""
rm b file
+mkdir -p b/c/d/
+mkdir a/
+echo a > b/c/d/file
+testing "-D b/c/d/file a/" "cp -D b/c/d/file a/ && cat a/b/c/d/file" "a\n" "" ""
+rm -rf a/
+rm -rf b/
+
# cp -r ../source destdir
# cp -r one/two/three missing
# cp -r one/two/three two
diff --git a/toys/posix/cp.c b/toys/posix/cp.c
index f70c0501..06f1ff92 100644
--- a/toys/posix/cp.c
+++ b/toys/posix/cp.c
@@ -407,7 +407,7 @@ void cp_main(void)
if (*--trail == '/') *trail = 0;
if (destdir) {
- char *s = FLAG(D) ? dirname(src) : getbasename(src);
+ char *s = FLAG(D) ? src : getbasename(src);
TT.destname = xmprintf("%s/%s", destname, s);
if (FLAG(D)) {
--
2.20.1
_______________________________________________
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net
