[Toybox] dirtree_notdotdot() when new->parent.

Rob Landley rob at landley.net
Mon Nov 21 14:56:05 PST 2016


When you do things like "chown -R root:root ." it should recurse through
the explicit "." on the command line, but not through the . and ..
entries in each directory. This means when !node->parent you force
dirtree_notdotdot(node) to be true.

Right now the callers are doing this check, but some are missing it (ala
the recent chmod fix reported by Josh Gao), which implies the callback
itself should be doing it.

The problem is, the callers vary. Some are straightforward "if
(!new->parent && dirtree_notdotdot(new)) return 0;" easy to convert, but
others are like du.c where they have special behavior at the top level
and then the else is the dirtree_notdotdot() check, so it would be
checking them either way. And "rm -rf ." is explicitly supposed to
filter out "." and ".." at the top level because posix says so.

If we _do_ need to skip the top level, then we need a function that
checks for "." and ".." without checking !parent, and there already is
one but it's static. So expose that (and swap it to isdotdot() so it's
more easily distinguished. Note: dirtree_notdotdot() acts as a default
callback that reads a directory tree and returns a tree of nodes.)

So if we've got three different types of behavior here, we've got to
audit all the users, ala "grep -l dirtree toys/*/*.c":

rm.c: need to skip top level "." and "..", so change to isdotdot()

ps.c: doesn't call notdotdot, uses !atol() to filter instead. There's
some stuff to do here but that's the DIRTREE_NOSTAT patched after thsi one.

ls.c: this one has always turned the dirtree infrastructure inside out
because it wants to do stuff in a different order than simple recursive
traversal. (ls -lR displays breadth first instead of depth first, so if
you don't want to buffer the entire tree before displaying anything you
can't do simple one pass recursion but have to come _back _ to the
directories to descend into them.)

That said, there are two calls to dirtree_notdotdot(): filter() calls it
but the nodes it's called on always have a parent (indir in
listfiles()), and listfiles() calls it near the end when recursing into
child nodes, and does an explicit test for !indir_parent(), but that's
needed because the notdotdot call is gated by FLAG_R.

So no change there.

grep.c: explicit new->parent test, can be removed.

find.c: if (new->parent) { if (!dirtree_notdotdot(new)) return 0; But
there's a second if() in those curly brackets, so removing it wouldn't
actually buy us anything. Still needs the test.

du.c: another explicit !parent test that triggers additional behavior so
has to stay.

cp.c: Ha! Doing it wrong:

  mkdir sub; cd sub; touch blah; cp -a . ../boing; ls -l boing
  ls: cannot access ../boing: No such file or directory

(Does enabling the !parent check impact "install"? Hmmm, that accept
directories as source, only target, so I don't think so.)

chmod.c: explicit parent test, can be removed.

chgrp.c: doing it wrong, fixed by this.

tar.c: explicit test, can be removed

modprobe.c: this is in pending for a reason. it's doing dirtree_read on
non-absolute paths for modules.symbols and modules.alias, but it's just
possible the "include" argument could point to "." in which case yes,
this would fix it. (Otherwise, silently ignored.)

mke2fs.c: fixed by this, but the code is still only 50% there. (I should
try to get back to that someday...)

mdev.c: also a stub, and doesn't use notdotdot or an equvalent.

lsof.c: no impact here but the NOSTAT change should probably be added here.

diff.c: explicit test, removed.

taskset.c: task_callback() is never called with "." or ".." so doesn't
care, but this is _also_ looking at /proc/[0-9]* and it sounds like I
should have a wrapper callback that does that. Hmmm...

sysctl.c: called with /proc... paths, unaffected.

switch_root.c: del_node() is called with / so unaffected.

modinfo.c: Can't call it with "." or ".." at the top level, but it does
the dirtree_notdotdot() call at the end returning its value, which is
slightly wrong because that'll do DIRTREE_SAVE which is a memory leak here.

lsusb: this hand-codes notdotdot, and the call is hardwired to a deep
sysfs subdirectory... and has an "snprintf(toybuf, "%s/%s",
sizeof(toybuf), vfs_filename, "hardwired string"); Right. That last
string should be in the format string, and vfs_filename maxes out at 256
bytes due to vfs limitations so _can't_ overflow unless the kernel is
compromised so this is some knee-jerk "never use sprintf, always use
snprintf, because reasons!" Sigh...

lspci.c: doesn't use notdotdot, just recurses one level from the parent.

lsattr.c: Whole command needs cleanup. Sigh. (I have a todo item to that
effect.) But in the context of _this_ review, the chattr update_attr()
callback was skipping top level "." or ".." (probably wrong?) and
retell_dir() has very weird plumbing. It's explicitly checking for and
recursing past a top level directory... execpt it's COMEAGAIN so should
call it a a second time? (Why? Hmmm... because the later recursion is
only with -R.) _WHY_ is this using COMEAGAIN?

Um, _wow_ lsattr is a crappy command... possibly I should do its larger
cleanup first, because tweaking the flow control here without
understanding the full insanity seems likely to break stuff.

losetup.c: doesn't use notdotdot, unaffected.

hwclock.c: doesn't use notdotdot, unaffected. (But once _again_ doing
the snprintf thing on node->name which _can't_ be longer than 255 bytes
due to the Linux vfs. A filesystem with individual filenames longer than
256 bytes each would be incompatible with the Linux virtual filesystem
layer. Paths can be arbitrarily long but path components cannot, it's
NAME_MAX in #include <linux/limits.h>.)

And while we're at it, can the RTC_RD_TIME ioctl from the kernel return
a time that mktime will then barf on? (I'm guessing "probably", but... ew?)

chcon.c: fixed by this. (I'm assuming if you "chcon -R ." you want it to
do that?)

acpi.c: hand-codes notdotdot (but it's all hardcoded /sys paths so eh).

netstat.c: it's doing the /proc/[0-9]* thing again. Not impacted.

So changes to: rm grep cp chmod tar diff modinfo lsusb hwclock

fixed by this: cp chgrp modprobe chcon

Probably broken by this but needing its own cleanup pass: chattr/lsattr

Other potentially common infrastructure needed:

Immedately recurse down skipping top level directory: lsattr lsof
taskset losetup lsusb lspci hwclock acpi netstat hwclock acpi...

Traverses /proc/[0-9]*: ps lsof task_callback netstat

Rob



More information about the Toybox mailing list