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@driftwood:~/toybox/toybox$ mkdir -p /home/landley/walrosity
landley@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
_______________________________________________
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net
