[Toybox] integration of SMACK

[José Bollo]

Le vendredi 08 mai 2015 à 20:13 -0500, Rob Landley a écrit :
(snip)

> I didn't make this change.

yes

(snip) 

> These are not my comments.

yes again

> Ok, the patch attributed to me is not the patch I sent you.

yes I reworked it

(snip)

> >  // 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?

Be happy, I finally discovered how to proceed. For my first trials I was
just to busy to get enough time.

> 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.

I still miss the point. Do you really want to collapse? It is not needed
you know.

(snip)

> > -    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.

Very good remarks.

Getting extended attributes on a descriptor opened with O_PATH fails.
That is explicitely writen on the man page of open that O_PATH doesn't
doesn't support fgetxattr: "The file itself is not opened, and other
file  operations  (e.g., read(2), write(2), fchmod(2), fchown(2),
fgetxattr(2), mmap(2)) fail with the error EBADF"

For the read mode of the files, I made the test. In the case where there
no read permission on a file, the functions of the family getxattr fail
with EPERM. That behaviour is not documented in the man page. But the
semantic is correct.

But the semantic is little different because opening for read fails with
EACCESS instead of EPERM.


> 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.

Yes it is not well defined. In the case of smack, the best would be to
follow the link if no security attribute is defined on it to print the
context of the target instead of some obscur "?".

(snip)
 
> > -  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.

What is the goal? less code? you are right. faster execution? depends on
cpu and data. human readability? unrolloing is better but that's so
small.

(snip)

> > @@ -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]

It makes me crazy ;)

> > -      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?

Did you checked the use of setvbuf(stdout, NULL, _IOLBF, 0) or
setlinebuf? 

# grep -r setlinebuf
toys/pending/dhcp.c:  setlinebuf(stdout);
toys/pending/dhcpd.c:  setlinebuf(stdout);
toys/pending/pgrep.c:  setlinebuf(stdout);

yes. so let use it.

> 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.)

yes I see.

(snip)

> And we've reached the end!

Thank you Rob I'll resubmit something soon.

Best regards
José Bollo


 1431349100.0