[Toybox] integration of SMACK
Rob Landley
rob at landley.net
Fri May 8 18:13:04 PDT 2015
On 05/05/2015 10:18 AM, José Bollo wrote:
> Hi all,
>
> I rebased the works made for Smack on the head of toybox. This fruit can
> be picked here.
>
> https://github.com/jobol/toybox/tree/smack-7
I'm a ways behind, but here's the notes I made on the last commit
I did do for this (git 06d378325b9e). Quite probably you've already
discussed this later in the thread, but here's what I had to say at
the time:
> commit a67d503979aeeaa3b2bd57679384f0ec53652e47
> Author: Rob Landley <rob at landley.net>
> Date: Thu Apr 30 10:31:52 2015 +0200
>
> ls: enhance smack LSM handling
>
> Change-Id: I538c9caa627336aba219620d3118626e8e198316
>
> diff --git a/lib/lib.c b/lib/lib.c
> index ab773b5..40927c3 100644
> --- a/lib/lib.c
> +++ b/lib/lib.c
> @@ -282,7 +282,7 @@ long atolx_range(char *numstr, long low, long high)
>
> int numlen(long l)
> {
> - int len = 0;
> + int len = !l;
I didn't make this change.
> --- a/lib/portability.h
> +++ b/lib/portability.h
> @@ -226,6 +226,14 @@ ssize_t getline(char **lineptr, size_t *n, FILE *stream);
> #define O_CLOEXEC 02000000
> #endif
>
> +#ifndef O_PATH
> +#define O_PATH 010000000 // linux value. why?
> +#endif
> +
> +#ifndef O_NOATIME
> +#define O_NOATIME 01000000 // linux value. why?
> +#endif
> +
These are not my comments.
Ok, the patch attributed to me is not the patch I sent you.
Right. Backing up. I'm going to commit my patch to my tree (on top of your
earlier patch I already committed), and then diff your tree vs mine to
see what your most recent round of changes did.
> --- tizen/toys/posix/ls.c 2015-04-30 14:15:02.618277194 -0500
> +++ toybox/toys/posix/ls.c 2015-04-29 21:55:06.308261647 -0500
> @@ -137,33 +137,35 @@
> return gr ? gr->gr_name : TT.gid_buf;
> }
>
> +static int numlen(long long ll)
> +{
> + return snprintf(0, 0, "%llu", ll);
> +}
> +
In the -si cleanup I noticed that numlen isn't used by anything outside
of ls.c, because everything else leverages snprintf's ability to count the
digits itself, so I don't need to implement my own function for this. But
ls uses it enough times that an abbeviated one argument version still makes
sense as a local cleanup, so I did a local version.
But I forgot to check in the removal from lib.c and lib.h, so you...
reverted the change I made.
I just checked in the lib.c and lib.h removal instead.
> // measure/print smack attributes
> -// pad: 0 just measure; <0 left justified; >0 right justified
> -static unsigned zmack(int dirfd, struct dirtree *dt, int pad)
> +// lr = 0 just measure, 1 left justified, 2 right justified
> +static unsigned zmack(struct dirtree *dt, int pad, int lr)
1) dirfd should be the return value of dirtree_parentfd(dt) and we're passing
in dt as an argument, so we can just call the function? Why pass it as a
third argument?
2) pad is the _amount_ to pad by. lr is the direction of left or right
justification, with 0 mean to just measure.
In theory I could collapse pad and lr together as a cleanup, I added "pad"
because totals[7] isn't a global. But if I did collapse them together,
it would still need the _amount_ to pad by (with a negative amount or a
zero amount having specific meanings). I.E. lr's API for pad would still
be wrong after such a cleanup.
> {
> - unsigned len = 0;
Note that you've made len unsigned here. We'll return to this in a moment.
> + char buf[SMACK_LABEL_LEN+1];
> + int fd = openat(dirtree_parentfd(dt), dt->name, O_PATH|O_NOFOLLOW);
> + ssize_t len = 1;
> +
> + strcpy(buf, "?");
> + if (fd != -1) {
> + len = fgetxattr(fd, XATTR_NAME_SMACK, lr?buf:0, lr?SMACK_LABEL_LEN:0);
> + close(fd);
>
> - if (CFG_LS_SMACK) {
Having the if () on the callers makes it fairly easy for the compiler to
optimize away this entire function when nobody's calling it. Having the
if () inside the function means it only reliably optimizes it away if it can
inline every use of it.
> - char buf[SMACK_LABEL_LEN+1];
> - int fd = openat(dirfd, dt->name, O_NOATIME/*|((toys.optflags & FLAG_L) ? 0 : O_NOFOLLOW)*/);
1) This will fail if you ls a file that's chmod 000, or a file that doesn't
belong to you. The O_PATH lets you access metadata for files you can't
access contents of.
2) I'm all for having -L stomp O_NOFOLLOW, but checking in commented out
code? Either do it or don't do it, the above is far too easy to miss.
> - if (fd != -1) {
> - ssize_t sts = fgetxattr(fd, XATTR_NAME_SMACK, buf, SMACK_LABEL_LEN);
Feeding in a 0 for buf and len was so the measure call didn't actually copy
the data. Does it still measure if you give it a valid but too-small
buffer? The man page says:
RETURN VALUE
On success, a positive number is returned indicating the size of the
extended attribute value. On failure, -1 is returned and errno is set
appropriately.
If the size of the value buffer is too small to hold the result, errno
is set to ERANGE.
Is the second part, where it sets errno, considered a failure nor not?
(I dunno. That's why I didn't do what your code does. Presumably you've
tried it.)
Also, if errno is -ENOTSUP should we still print ? for these fields?
> - close(fd);
> - if (0 <= sts && sts <= SMACK_LABEL_LEN)
> - len = (unsigned)sts;
> - }
Ok, remember the "note len is unsigned here" above? You're typecasting a
value that will be automatically converted when you asssign it anyway. So
this typecast can't POSSIBLY accomplish anything.
But let's assume that len wasn't unsigned. You just checked whether or not
the value was within a small positive range. If sts <= 0 or
sts > SMACK_LABEL_LEN your assignment won't trigger, so even in that case,
typecasting it to unsigned accomplishes what exactly?
Are you worried about integer overflow? According to this:
https://github.com/smack-team/smack/blob/master/libsmack/sys/smack.h#L35
So the high end of your range is 255.
> - while (len != 0 && buf[len-1] == 0) len--; // removes trailing nulls
Can you really have trailing nulls in an extended attribute? The API told us
the length of the data it returned, if some of that data is NUL bytes this
is NOT valid data...?
(Is it or isn't it nul terminating the returned data? You're assigning your
own null terminator, but not feeding it a buffer size that includes space for
a nul terminator. Pick one?)
> - if (len == 0) buf[len++] = '?'; // makes unknown label
> - buf[len] = 0; // enforce ending null
> - if (pad) printf("%*s ", pad, buf);
I'm reading along in sequence so I haven't see what changes you'e made to
the caller yet, but from this I'm assuming that you're feeding in a negative
pad when you want it left justified, and handling your own left-side padding
in the caller when necessary?
> + if (len<1 || len>SMACK_LABEL_LEN) len = 0;
> + else buf[len] = 0;
> }
> + if (lr) printf(" %*s "+(lr==2), pad*((2*(lr==1))-1), buf);
>
> return len;
> }
>
> // Figure out size of printable entry fields for display indent/wrap
>
> -static void entrylen(int dirfd, struct dirtree *dt, unsigned *len)
> +static void entrylen(struct dirtree *dt, unsigned *len)
> {
> struct stat *st = &(dt->st);
> unsigned flags = toys.optflags;
> @@ -184,8 +186,10 @@
> len[5] = numlen(major(st->st_rdev))+5;
> } else len[5] = numlen(st->st_size);
> }
> +
> len[6] = (flags & FLAG_s) ? numlen(st->st_blocks) : 0;
> - len[7] = (CFG_LS_SMACK && (flags & FLAG_Z)) ? zmack(dirfd, dt, 0) : 0;
> +
> + if (CFG_LS_SMACK && (flags & FLAG_Z)) len[7] = zmack(dt, 0, 0);
> }
Again, you can call dirtree_parentfd(dt) on the dt we pass in...
> static int compare(void *a, void *b)
> @@ -289,8 +293,8 @@
>
> static void listfiles(int dirfd, struct dirtree *indir)
> {
> - struct dirtree *dt, **sort;
> - unsigned long dtlen, ul = 0;
> + struct dirtree *dt, **sort = 0;
> + unsigned long dtlen = 0, ul = 0;
> unsigned width, flags = toys.optflags, totals[8], len[8], totpad = 0,
> *colsizes = (unsigned *)(toybuf+260), columns = (sizeof(toybuf)-260)/4;
>
> @@ -314,9 +318,16 @@
> // Copy linked list to array and sort it. Directories go in array because
> // we visit them in sorted order.
>
> - for (dtlen = 0, dt = indir->child; dt; dt = dt->next) dtlen++;
> - sort = xmalloc(dtlen * sizeof(void *));
> - for (dtlen = 0, dt = indir->child; dt; dt = dt->next) sort[dtlen++] = dt;
> + for (;;) {
> + for (dt = indir->child; dt; dt = dt->next) {
> + if (sort) sort[dtlen] = dt;
> + dtlen++;
> + }
> + if (sort) break;
> + sort = xmalloc(dtlen * sizeof(void *));
> + dtlen = 0;
> + continue;
> + }
Alright, I'll accept that as a cleanup. (My reluctance to repeat the for (;;)
with the exact same tests led to unnecessary awkwardness there.
Except the unrolled version has three identical assignments, two identical
increments, and a repeated test. Grrr. How about:
for (sort = 0;;sort = xmalloc(dtlen * sizeof(void *))) {
for (dtlen = 0, dt = indir->child; dt; dt = dt->next, dtlen++)
if (sort) sort[dtlen] = dt;
if (sort) break;
}
Which is too clever. Needs a comment.
> // Label directory if not top of tree, or if -R
> if (indir->parent && (!indir->extra || (flags & FLAG_R)))
> @@ -333,12 +344,12 @@
>
> qsort(sort, dtlen, sizeof(void *), (void *)compare);
> for (ul = 0; ul<dtlen; ul++) {
> - entrylen(dirfd, sort[ul], len);
> + entrylen(sort[ul], len);
> for (width = 0; width<8; width++)
> if (len[width]>totals[width]) totals[width] = len[width];
> + totpad = totals[1]+!!totals[1]+totals[6]+!!totals[6]+totals[7]+!!totals[7];
> blocks += sort[ul]->st.st_blocks;
> }
> - totpad = totals[1]+!!totals[1]+totals[6]+!!totals[6]+totals[7]+!!totals[7];
Ok, you're right. That doesn't need to be in the loop.
> if (flags & (FLAG_l|FLAG_o|FLAG_n|FLAG_g|FLAG_s) && indir->parent)
> xprintf("total %llu\n", blocks);
> }
> @@ -357,7 +368,7 @@
>
> memset(colsizes, 0, columns*sizeof(unsigned));
> for (ul=0; ul<dtlen; ul++) {
> - entrylen(dirfd, sort[next_column(ul, dtlen, columns, &c)], len);
> + entrylen(sort[next_column(ul, dtlen, columns, &c)], len);
> *len += totpad;
> if (c == columns) break;
> // Expand this column if necessary, break if that puts us over budget
> @@ -387,7 +398,7 @@
> TT.nl_title=1;
>
> // Handle padding and wrapping for display purposes
> - entrylen(dirfd, sort[next], len);
> + entrylen(sort[next], len);
> if (ul) {
> if (flags & FLAG_m) xputc(',');
> if (flags & (FLAG_C|FLAG_x)) {
More adding unnecessary parameter.
> @@ -413,33 +424,34 @@
>
> mode_to_string(mode, perm);
>
> - // Coerce the st types into something we know we can print.
> - printf("%s% *ld ", perm, totals[2]+1, (long)st->st_nlink);
> + if (flags&FLAG_o) grp = grpad = toybuf+256;
> + else {
> + if (flags&FLAG_n) sprintf(grp = thyme, "%u", (unsigned)st->st_gid);
> + else strwidth(grp = getgroupname(st->st_gid));
> + grpad = toybuf+256-(totals[4]-len[4]);
> + }
So if flag -o is set, we ignore flag -n. But -n, -o, and -no produce three
different types of output?
Ah, I see. The [-1Cglmnox] up top will switch off previous entries in the -n -o
group. But ls -l1 is very much not the same as ls -1. So you've caught an
existing bug, albeit by going further in the wrong direction.
Ok, -gon switch off fields from -l, so -lg and -gl work the same. So
if gon are set, switch on l. Heh: ls -gCl remembers -g when -l switches
back to that mode, but -gC and -Cg take the last one.
Grr. How do you represent this? "ls -1Cl" and "ls -lC1" produce different
output, but "ls -l1" collapses together. The -1 isn't bringing back the -l
the way the -l is bringing back the -g.
Ok, what I care about are _unique_ types of output. Options -ong act as
modifiers to -l, and -l sort of acts as a modifier to -1. So really we've
got -Cx mode, and -1long mode, and the two groups are exclusive.
I'm trying to think of a better way of expressing that than:
[-Cxm1][-Cxml][-Cxmo][-Cxmn][-Cxmg]
> - if (!(flags&FLAG_g)) {
> + if (flags&FLAG_g) usr = upad = toybuf+256;
> + else {
> + upad = toybuf+255-(totals[3]-len[3]);
> if (flags&FLAG_n) sprintf(usr = TT.uid_buf, "%u", (unsigned)st->st_uid);
> else strwidth(usr = getusername(st->st_uid));
> - printf("%*s ", -(int)totals[3], usr);
> }
>
> - if (!(flags&FLAG_o)) {
> - if (flags&FLAG_n) sprintf(grp = thyme, "%u", (unsigned)st->st_gid);
> - else strwidth(grp = getgroupname(st->st_gid));
> - printf("%*s ", -(int)totals[4], grp);
> - }
> + // Coerce the st types into something we know we can print.
> + printf("%s% *ld %s%s%s%s", perm, totals[2]+1, (long)st->st_nlink,
> + usr, upad, grp, grpad);
Diff really did not help me parse that.
I _think_ the point of this is to replace the explicit padding (possibly
there to try to reduce the number of write() calls the printf does under the
covers? I forget) into printf %*s padding? If so, why didn't the declarations
of upad and grpad go away?
Still, assuming that's what this is for, I can take a stab at that.
> - if (CFG_LS_SMACK && (flags & FLAG_Z))
> - zmack(dirfd, sort[next], -(int)totals[7]);
> + if (CFG_LS_SMACK && (flags & FLAG_Z)) zmack(sort[next], totals[7], 1);
Yay! Finally a typecast that might actually be necessary...
> if (S_ISCHR(st->st_mode) || S_ISBLK(st->st_mode))
> printf("% *d,% 4d", totals[5]-4, major(st->st_rdev),minor(st->st_rdev));
> - else printf("%*"PRId64, totals[5], (int64_t)st->st_size);
> + else printf("% *"PRId64, totals[5]+1, (int64_t)st->st_size);
Without that space we left pad with zeroes. (I keep accidentally sticking it
on strings, where it's not needed, but for numbers it _is_ needed.)
(And while we're at it: %lld is at least 64 bit in LP64. They don't rule out
it being 128 bit, but nobody does that yet. So this PRId64 nonsense isn't
actually necessary, it can just be "% *lld" and a typecast to long long.)
> tm = localtime(&(st->st_mtime));
> strftime(thyme, sizeof(thyme), "%F %H:%M", tm);
> xprintf(" %s ", thyme);
> - } else if (CFG_LS_SMACK && (flags & FLAG_Z))
> - zmack(dirfd, sort[next], totals[7]);
> + } else if (CFG_LS_SMACK && (flags & FLAG_Z)) zmack(sort[next], totals[7],2);
>
> if (flags & FLAG_color) {
> color = color_from_mode(st->st_mode);
And we've reached the end!
Rob
1431133984.0
More information about the Toybox
mailing list