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 <[email protected]> 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 a63a62823c8e47df54538d09b049bb8bc7c00c4d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jarno=20M=C3=A4kip=C3=A4=C3=A4?= <[email protected]>
Date: Sat, 15 Feb 2020 13:31:48 +0200
Subject: [PATCH] cp: fix -D (--parents) (REWORK)
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. strdup should be used with
getbasename() since path under src should not change here
either but path under dest should contain basename.
github.com/landley/toybox/issues/165
---
tests/cp.test | 7 +++++++
toys/posix/cp.c | 10 ++++++----
2 files changed, 13 insertions(+), 4 deletions(-)
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..8c47b31c 100644
--- a/toys/posix/cp.c
+++ b/toys/posix/cp.c
@@ -407,18 +407,20 @@ void cp_main(void)
if (*--trail == '/') *trail = 0;
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;
errno = EXDEV;
--
2.20.1
_______________________________________________
Toybox mailing list
[email protected]
http://lists.landley.net/listinfo.cgi/toybox-landley.net
