[Toybox] cp.c

[Rob Landley]

On 3/11/21 10:30 AM, enh via Toybox wrote:
> i don't understand this code in cp.c, specifically the marked line. because of
> the `errno = EXDEV` on the first line of this snippet, and the absence of any
> `errno = 0` in between, what was the `if (errno &&` for?

Back in 2013 the code originally looked like:

    errno = EXDEV;
    if (toys.which->name[0] == 'm') rc = rename(src, TT.destname);

    // Skip nonexistent sources
    if (rc) {
      if (errno != EXDEV ||
        !(new = dirtree_add_node(0, src, !(toys.optflags & (FLAG_d|FLAG_a)))))
          perror_msg("bad '%s'", src);
      else dirtree_handle_callback(new, cp_node);
    }

The if (errno && stuff) was added by commit 3b9cfa70db in 2019, a commit which
should definitely have set errno to 0 at some point, but did not. (Sorry, I
should have caught that.)

That patch was actually wrong for multiple reasons, because it doesn't check the
--trail going off the START of the string either, and it's modifying environment
data and thus screwing up what "ps" sees...

A quick guess at fixing at least a couple of those issues:

diff --git a/toys/posix/cp.c b/toys/posix/cp.c
index 56fbadf9..24b9d777 100644
--- a/toys/posix/cp.c
+++ b/toys/posix/cp.c
@@ -412,11 +412,11 @@ 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], *trail;
     int rc = 1;

-    while (*++trail);
-    if (*--trail == '/') *trail = 0;
+    if (!(trail = strrchr(src, '/')) || trail[1]) trail = 0;
+    else while (trail>src && *trail=='/') *trail-- = 0;

     if (destdir) {
       char *s = FLAG(D) ? src : getbasename(src);
@@ -453,7 +453,7 @@ void cp_main(void)
         if (exists && no_clobber) rc = 0;
       }
       if (rc) rc = rename(src, TT.destname);
-      if (errno && !*trail) *trail = '/';
+      if (trail) trail[1] = '/';
     }

     // Copy if we didn't mv, skipping nonexistent sources

But could I get a second opinion on that since I obviously flubbed allowing this
patch in last time? My blog suggests I was hip-deep in doing the first round of
modern shell stuff. Twitter suggests... oh hey, there's my old tweet explaining
how to fix the audio output on my laptop that's been broken for weeks now:

  https://twitter.com/landley/status/1142847348198039553

> it seems from the history like the EXDEV assignment was for the later test of
> `errno != EXDEV` and is really meant to be a "did we successfully move?". i
> don't think that's still valid now the force/clobber code has been added?

I think the rest of it's ok?

The if (rc) test still gets set to 0 if rename() failed or if we answered "n" to
yesno, or if -n and dest exists. So we only go into the original errno check
when we need to fallback from mv to cp, and then it checks "is this an error
other than "not same device", which is the only failure we should accept as
"couldn't rename so try to copy contents".

The if (MV) stanza has to exit past if (rc) rc = rename() so either rc is 0 or
rename() sets errno. So that first assignment only matters to the "copy if we
didn't mv" errno test if we either skipped if (MV) and are looking at the
manually set errno = EXDEV, or if rename() failed and set errno.

The problem with the other errno test that got added is he assumed that a
successful function will set it to zero rather than leaving it unchanged.

Rob