`mv` and `cp` builtins cannot move or copy directories into /tmp or /dev/shm

[JohnoKing]

This gave me grief after a script failed when prepending PATH=/opt/ast/bin:$PATH. Reproducer:

# run on Linux at ksh repo root; /tmp and /dev/shm are both mounted to tmpfs
$ /opt/ast/bin/mv bin /dev/shm
mv: bin: /dev/shm/bin read error [Is a directory]
$ /opt/ast/bin/cp -r bin /dev/shm
cp: /dev/shm/bin: not a directory -- bin ignored
cp: /dev/shm/bin/package: cannot write [Not a directory]
cp: /dev/shm/bin/shtests: cannot write [Not a directory]
$ /opt/ast/bin/mv bin /tmp    
mv: bin: /tmp/bin read error [Is a directory]

Code where the error occurs:

ksh/src/lib/libcmd/cp.c

Lines 603 to 615 in 68fe82b

	if (sfmove(ip, op, (Sfoff_t)SFIO_UNBOUND, -1) < 0)
	n |= 3;
	if (!sfeof(ip))
	n |= 1;
	if (sfsync(op) || state->sync && fsync(wfd) || sfclose(op))
	n |= 2;
	if (sfclose(ip))
	n |= 1;
	if (n)
	{
	error(ERROR_SYSTEM|2, "%s: %s %s error", ent->fts_path, state->path, n == 1 ? ERROR_translate(0, 0, 0, "read") : n == 2 ? ERROR_translate(0, 0, 0, "write") : ERROR_translate(0, 0, 0, "io"));
	return 0;
	}

I was able to trace the error as occurring after sfmove() fails. I haven't been able to debug it any further.

ksh93v- has some sort of bugfix applied to this area. I tried backporting that but it didn't help.

ksh93v-cp-mv-backport.patch

diff --git a/src/lib/libcmd/cp.c b/src/lib/libcmd/cp.c
index 2087baef4..69a368738 100644
--- a/src/lib/libcmd/cp.c
+++ b/src/lib/libcmd/cp.c
@@ -573,7 +573,7 @@ visit(State_t* state, FTSENT* ent)
 		{
 			int	rfd = -1;
 			int	wfd = -1;
-			if (ent->fts_statp->st_size > 0 && (rfd = open(ent->fts_path, O_RDONLY|O_BINARY|O_cloexec)) < 0)
+			if ((!S_ISREG(ent->fts_statp->st_mode) || ent->fts_statp->st_size > 0) && (rfd = open(ent->fts_path, O_RDONLY|O_BINARY|O_cloexec)) < 0)
 			{
 				error(ERROR_SYSTEM|2, "%s: cannot read", ent->fts_path);
 				return 0;
@@ -581,11 +581,11 @@ visit(State_t* state, FTSENT* ent)
 			else if ((wfd = open(state->path, (st.st_mode ? (state->wflags & ~O_EXCL) : state->wflags)|O_cloexec, ent->fts_statp->st_mode & state->perm)) < 0)
 			{
 				error(ERROR_SYSTEM|2, "%s: cannot write", state->path);
-				if (ent->fts_statp->st_size > 0)
+				if (rfd >= 0)
 					close(rfd);
 				return 0;
 			}
-			else if (ent->fts_statp->st_size > 0)
+			else if (rfd >= 0)
 			{
 				if (!(ip = sfnew(NULL, NULL, (size_t)SFIO_UNBOUND, rfd, SFIO_READ)))
 				{

[phidebian]

Just curious, being ignorant here, how to you build libcmd/ builtins?
My build don't produce them

[JohnoKing]

For ksh binaries dynamically linked against libast, all of the builtins are available (try bin/package use, vide f15e2c4). Such a binary is placed by the build system at ./arch/linux.i386-64/dyn/bin/ksh.

For ksh binaries statically linked against libast (so ./arch/linux.i386-64/bin/ksh), you must enable the relevant SHOPT manually:

ksh/src/cmd/ksh93/SHOPT.sh

Line 10 in 9e2e6ca

SHOPT ALL_LIBCMD=0 # include all libcmd path-bound built-ins in statically linked ksh

[phidebian]

Ha ok, that's true that my build do build libcmd.so
I'd like to dynload it, but I got

$ builtin -f /d1/ksh-phideb/arch/linux.i386-64/dyn/lib/libcmd.so 
builtin: adding built-ins not supported
Usage: builtin [-dls] [-f lib] [pathname ...]
 Help: builtin [ --help | --man ]

I then discover that my SHOPT.sh don't have SHOPT_DYNAMIC set to 1
So the next question is how do I turn SHOPT_DYNAMIC=1
I tried CCFLAGS='-DSHOPT_DYNAMIC=1' no success
I tried echo "SHOPT DYNAMIC=1" >>SHOPT.sh still no success

Thanx in advance

[phidebian]

Ok I got it. Sounds like I got nothing to setup.
My build do build

• ./arch/linux.i386-64/bin/ksh
• ./arch/linux.i386-64/dyn/bin/ksh

Never noticed the later, that's the one to use do dynamic builtin loading.
I got to run it with

$ LD_LIBRARY_PATH=./arch/linux.i386-64/dyn/lib \
  ./arch/linux.i386-64/dyn/bin/ksh

TC$ builtin | grep cp
/opt/ast/bin/cp

TC$ builtin -f libcmd.so cp

TC$ builtin | grep cp      
/opt/ast/bin/cp
cp

[phidebian]

Oops didn't saw you mentioned ./arch/linux.i386-64/dyn/bin/ksh

I am not use to this one :)
Thanx

[phidebian]

Just posted PR #878 to address this.

As explain in the commit message, the main bug is from mv that is unable to cope with xdev FS (cross-mounted) i.e src... are not located on the same FS as the dst.
The but was sever in the sense that mv bin /otherfs/ will leave a lefter regular file name bin.

Then any attempt to /opt/ast/bin/cp -r bin somewhere will fail as bin is not a directory.

I wonder if those utilities are worth maintaining, the cp and mv are very primitive, don't offer the rich set of options we got on modern OS's and indeed are bugged.

For long lived of such commands, i.e cp, xdev mv on large hierarchy, the fork/exec cost of OS cp(1) or mv(1) must be negligible.

The builtin could be usefull though for short lived cp or mv avoiding the cost of a fork/exec in a perf path (inner loop). In such case a script writer could do elementaty copy/rename with a builtin that should be called something else like uni_cp, uni_mv, they would not accept hierarchy recursion options...

Anyway, I made a fix/work around, in case this bug impair you ongoing dev, not sure this PR will be accepted, yet it is relatively harmless

[JohnoKing]

I wonder if those utilities are worth maintaining, the cp and mv are very primitive

I'm inclined to agree with the notion of deleting the cp and mv builtins. Modern implementations of cp can be a heck of a lot faster than the old AST version. Take the following comparison for example:

# Use any large file (I'm copying a virtual machine disk for demonstration)
#
# libcmd cp (dev branch)
$ time /opt/ast/bin/cp /home/johno/VirtualBox\ VMs/Haiku/Haiku.vdi ~/Downloads

real	0m02.700023s
cpu	0m02.651301s


# uutils cp (v0.1.0)
$ time uu-cp /home/johno/VirtualBox\ VMs/Haiku/Haiku.vdi ~/Downloads

real	0m00.786799s
cpu	0m00.727251s


# BusyBox cp (v1.36.1)
$ time busybox cp /home/johno/VirtualBox\ VMs/Haiku/Haiku.vdi ~/Downloads

real	0m02.568440s
cpu	0m02.445647s


# GNU coreutils cp (v9.7)
$ time /bin/cp /home/johno/VirtualBox\ VMs/Haiku/Haiku.vdi ~/Downloads

real	0m00.734879s
cpu	0m00.730186s

In all examples on Linux the system's implementation is faster than the ksh builtin. This makes me seriously question the utility of the cp and mv builtins, since those were intended to increase performance when they were first implemented. Read:

ksh/src/cmd/ksh93/PROMO.mm

Lines 66 to 71 in 970812e

	Improved performance: KSH-93 executes many scripts faster
	than the System V Bourne shell. A major reason for this is
	that many of the standard utilities are built-in.
	To reduce the time to initiate a command, KSH-93 allows
	commands to be added as built-ins at run time
	on systems that support dynamic loading such as System V Release 4.

I'm personally in favor of removing them entirely to reduce maintenance burden. I'm not sure if that's a popular opinion though.

[phidebian]

May be an approach could simply to ignore this code, may be just advertising we don't maintain it, but just leave it there in case on depend on it.

Doing nothing (apart sayin we don't maintain it) also make it easy on our resources, because I imagine removing this mean tweak the build, the doc, the QA, so I would vote to simply officially say we don't maintain it.