Thanks for the review! Addresed all points. Should've written more test
cases and caught the need to handle empty string and more slashes. Yes,
musl rename() seems alright, the problem is instead with getbasename().
From fc92db71a1cc11cb152cb4aa90d589264a54c0ac Mon Sep 17 00:00:00 2001
From: Denys Nykula <[email protected]>
Date: Tue, 25 Jun 2019 01:53:04 +0300
Subject: [PATCH] Fix mv empty string and multiple slashes.
Following Rob's review tips, adjust the intermediate string when
building the destination path, and keep the source argument unmodified.
In response to one of review points, removing the slash can't happen
next to the rename call, has to preceed TT.destname derivation from
source using getbasename(). Given a trailing slash, the function returns
the portion of the path containing just the null character. That caused
original issue and is what I'm trying to handle.
Modprobe, netstat and sh expect a file and would fail even if
getbasename() handled the trailing slashes and resolved the directory
correctly. Cp is the only other user of this function, so the resolution
makes sense placed in cp, without changing other commands to free a
string after their basename lookup. One can extract this patch logic
into a new getbasename() implementation, when a need to map a path
argument to a destination name re-appears elsewhere.
---
tests/mv.test | 8 ++++++++
toys/posix/cp.c | 17 ++++++++---------
2 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/tests/mv.test b/tests/mv.test
index ed8922ac..06c93211 100755
--- a/tests/mv.test
+++ b/tests/mv.test
@@ -112,9 +112,17 @@ testing "dir1/dir2/ to ./" \
"mv dir1/dir2/ ./ && [ ! -e dir1/dir2 -a -d dir2 ] && echo yes" \
"yes\n" "" ""
rm -rf dir*
+mkdir -p dir1/dir2
+testing "dir1/dir2/// to .///" \
+ "mv dir1/dir2/// ./// && [ ! -e dir1/dir2 -a -d dir2 ] && echo yes" \
+ "yes\n" "" ""
+rm -rf dir*
testing "not/exists/ to ./" \
"mv not/exists/ ./ 2>&1 | grep -o not/exists/" \
"not/exists/\n" "" ""
+testing "'' to ''" \
+ "mv '' '' 2>&1 | grep -o \"bad ''\"" \
+ "bad ''\n" "" ""
touch file1 file2
chmod 400 file1 file2
diff --git a/toys/posix/cp.c b/toys/posix/cp.c
index 9989ed14..fe426a61 100644
--- a/toys/posix/cp.c
+++ b/toys/posix/cp.c
@@ -403,20 +403,20 @@ void cp_main(void)
// Loop through sources
for (i=0; i<toys.optc; i++) {
- char *src = toys.optargs[i], *trail = src;
+ char *src = toys.optargs[i];
int rc = 1;
- if (CFG_MV && toys.which->name[0] == 'm') {
- while (*++trail);
- if (*--trail == '/') *trail = 0;
- }
-
if (destdir) {
- char *s = (toys.optflags&FLAG_D) ? getdirname(src) : getbasename(src);
+ char *s = FLAG(D) ? getdirname(src) : getbasename(src), *s_, *trail;
+ if (!FLAG(D) && *(s = s_ = trail = xstrdup(*s ? s : src))) {
+ while (*++trail);
+ while (*--trail == '/') *trail = 0;
+ s = getbasename(s);
+ }
TT.destname = xmprintf("%s/%s", destname, s);
+ free(FLAG(D) ? s : s_);
if (FLAG(D)) {
- free(s);
if (!(s = fileunderdir(TT.destname, destname))) {
error_msg("%s not under %s", TT.destname, destname);
continue;
@@ -447,7 +447,6 @@ void cp_main(void)
if (exists && no_clobber) rc = 0;
}
if (rc) rc = rename(src, TT.destname);
- if (errno && !*trail) *trail = '/';
}
// Copy if we didn't mv, skipping nonexistent sources
--
2.21.0
_______________________________________________
Toybox mailing list
[email protected]
http://lists.landley.net/listinfo.cgi/toybox-landley.net
