[Toybox] Fixes to mkdir, chown, env

[Rob Landley]

On 08/30/2013 09:01:30 AM, Jacek Bukarewicz wrote:
> Hi
> 
> While testing toybox I encountered a few minor issues with toybox:
> 1) mkdir:
>     - according to the spec umask should be ignored when explicitly  
> setting permissions. I simply added umask(0) call to prevent umask  
> from affecting mode

There's a TOYBOX_UMASK flag you can add to newtoy that sets umask(0)  
before the command's main gets called. Alas, didn't turn out to be  
quite as universal as I thought because a lot of stuff wants to  
_selectively_ disable it, but it might apply here.

mkdir was implemented before we had the permission parsing logic in the  
library, so -m was added later and apparently never fully cleaned up.  
(hg log toys/mkdir.c)

>      - I believe that in mkdir.c line 60 the intent was to check if  
> flag "p" is not set.

save is initialized to 0 on line 46, and only set to *s on line 50  
inside
an if (toys.optflags&FLAG_p). Therefore the assignment at the end will
always assign 0 in the non-p case, so we don't explicitly need to test  
for
FLAG_p because it's implicit in the variable state.

(I try to come up with a test to demonstrate the failing behavior when  
dealing with code like that. Then I can show my change fixed the test  
case.)

>      - I added --mode longopt, but I noticed that it's not handled  
> correctly.
> I believe that's because gof.arg is assigned to "" (get_optflags  
> function in
> lib/args.c). gof.arg is incremented afterwards so it contains trash  
> then.
> I removed this line from lib/args.c and it works fine now

The problem is removing that line breaks other things. (There's both
"--mode=arg" and "--mode arg" and the short opt "-m arg" and "-marg"  
and of course the tar or ps style "tar xvjfC filename dirname" where C  
is not the option to f...

I redid this area recently due to Rich Felker asking for --color=auto  
to behave differently from --color with no argument, and I had a  
pending todo item that bare longopts with arguments weren't working  
right anyway, so...

   http://landley.net/hg/toybox/rev/1036

Hopefully it works now.

>     - I also added/modified a few mkdir  tests

Cool.

What's this bit?

@@ -42,19 +42,22 @@ static int do_mkdir(char *dir)
      return 1;
    }

-  for (s=dir; ; s++) {
+  // Skip leading / of absolute paths.
+  for (s=dir+1; ; s++) {
      char save=0;

-    // Skip leading / of absolute paths.
-    if (s!=dir && *s == '/' && (toys.optflags&FLAG_p)) {
+    if (*s == '/' && (toys.optflags&FLAG_p)) {

Is that forbidden?

   landley at driftwood:~/toybox/toybox$ mkdir -p /home/landley/walrosity
   landley at driftwood:~/toybox/toybox$ ls -ld /home/landley/walrosity
   drwxrwxr-x 2 landley landley 4096 Sep  4 04:45  
/home/landley/walrosity

Absolute paths work fine in the host version of this tool...? Standard  
filesystem permissions should prevent it from doing anything funky. The  
posix mkdir page doesn't contain the word "absolute'...

And this:

-    if ((toys.optflags&FLAG_m) && save != '/') mode = TT.mode;
+    if ((toys.optflags&FLAG_m) && save != '/') {
+      umask(0); // ignore umask for last path component
+      mode = TT.mode;
+    }

No, that's too simple. Posix says:

   -m  mode
     Set the file permission bits of the newly-created directory to the
     specified mode value. The mode option-argument shall be the same as
     the mode operand defined for the chmod utility. In the  
symbolic_mode
     strings, the op characters '+' and '-' shall be interpreted  
relative
     to an assumed initial mode of a= rwx; '+' shall add permissions to
     the default mode, '-' shall delete permissions from the default  
mode.

   -p
     Create any missing intermediate pathname components.
     For each dir operand that does not name an existing directory,  
effects
     equivalent to those caused by the following command shall occur:

     mkdir -p -m $(umask -S),u+wx $(dirname dir) &&
     mkdir [-m mode] dir

     where the -m mode option represents that option supplied to the
     original invocation of mkdir, if any.

Sigh. I'll take a swing at fixing this, but if the host umask is  
switching
off u+wx our behavior would still be wrong. (And I need a test for  
that.)

Heh, this code also predates teh automatic generation of FLAG_x macros,  
so
it was using hardwired constants and it looks like the conversion gave  
us
"toys.optflags&~FLAG_p" which is just another way of saying
toys.optflags&FLAG_m. Now the question is, SHOULD it be saying that?
FLAG_p allows failure, so I think it should be saying !(flag&FLAG_p) I
guess? (Rummage, rummage: ah, I didn't write commit 763, which added  
this.
Ok, that would explain why I don't remember doing something subtle here.
Not that I caught it before now, either... :)

And the existing mkdir tests are failing. Why...?

   VERBOSE=1 scripts/test.sh mkdir

Becuase we've implemented stat and are no longer using the host's  
version, and our stat -a is saying 0775 where the host one was saying  
775.

Sigh. And of course there's no spec for stat. Ok, try it on gentoo,  
ubuntu 8.04, and Red Hat 9 from a decade ago... not padding. We're  
padding to 4 digits, and should stop that.

Huh. The last test is failing, but passing for TEST_HOST=1. Why is the  
last test failing? Directory permissions are wrong. Why are permissions  
wrong? It's creating them right from the command line...

Because the rm -rf one just before that isn't deleting anything when  
using the toybox rm, but is when using the TEST_HOST rm. Not a mkdir  
issue, it's an rm issue!

It's weird because rm is returning 0 (no error) even though it didn't  
delete anything. And strace says it's traversing but not calling  
unlink. Why is not it not trying to delete? Because try->symlink is 1  
in directory entries which means nodelete flagged its parent (so we  
don't complain about being able to delete parent directories we  
couldn't remove the contents of), but how did that get flagged?

Ah, the dirtree infrastructure _also_ sets the parent's symlink=1 to  
specify child had an error (symlink doesn't have a valid nonzero value  
for directory entries), and the permissions of "124" don't let us  
enumerate the contents of the directory "two", so when it tries to stat  
the "." and ".." entries in there it flags it as having had an error:

   $ mkdir sub
   $ chmod 007 sub
   $ ls -l sub
   ls: cannot open directory sub: Permission denied

(Yeah, funky, but that's how linux does permissions. Permissions 077  
says anybody completely unrelated to us can read the file, but we  
can't...)

So directory traversal flags an error which rm sees as a nerror that  
was already handled and it's just suppressing further errors, so the  
exit code doesn't get set because it never _reported_ an error.

Ok, what's the right thing to do here? According to strace the host rm  
isn't chmoding entries, it's just trying to delete the directory even  
though it couldn't read its contents, and I think that's probably  
correct. But I don't want it failing to report an _actual_ failure to  
delete the innermost thing, and making it attempt to delete even in the  
do not report error case opens the possiblity of that failing for the  
leaf node and then not getting reported. Um. Ok, set it to a  
_different_ flag value.

Rob
 1378373360.0