Re: [Acl-devel] typo in acl_get_perm?

[Andreas Grünbacher]

2015-12-27 20:51 GMT+01:00 Corinna Vinschen <address@hidden>:
> On Dec 26 17:58, Mike Frysinger wrote:
>> On 26 Dec 2015 16:08, Corinna Vinschen wrote:
>> > On Dec 26 15:46, Andreas Grünbacher wrote:
>> > > 2015-12-26 14:34 GMT+01:00 Corinna Vinschen <address@hidden>:
>> > > > On Dec 26 11:57, Andreas Grünbacher wrote:
>> > > >> 2015-12-25 15:36 GMT+01:00 Corinna Vinschen <address@hidden>:
>> > > > [...]
>> > > > The man page doesn't make it clear that only a single permission bit is
>> > > > allowed as input.
>> > >
>> > > "The [...] function tests if the permission [...] is contained in the
>> > > [...] set."
>> > >
>> > > So again, what's unclear about that?
>> >
>> > Not trying to put too fine a point on it, but isn't the fact that I
>> > misunderstood an indicator that, perhaps, more than a single person
>> > might misunderstand it?  I'm not the only dumb person out there ;)
>>
>> he asked what's unclear in the quoted statement.  what's unclear about that 
>> ? ;)
>> -mike
>
> Ok, sorry if my mail was unclear on that point.  Let me try again:
>
> Perm is an acl_perm_t, which consists of any combination of ACL_READ|
> ACL_WRITE|ACL_EXECUTE.

Type acl_perm_t is actually not defined as a bit field; it is not a
requirement that the values of ACL_READ, ACL_WRITE, and ACL_EXECUTE
can be bitwise combined. Other implementations of this interface might
use bit numbers instead of bit masks, for example.

If acl_perm_t was defined as a bit field, there would be no point in
having functions like acl_add_perm, acl_delete_perm, acl_get_perm, or
a separate type acl_permset_t, and bit operations could be used
instead.

> It's not clear from the man page that only a
> single permission bit is allowed here, rather than an arbitrary
> combination of the above bits.

The acl_add_perm, acl_delete_perm, and acl_get_perm man pages all
refer to a permission (singular) for the acl_perm_t argument. This
seems to correctly describe the interface, it is not overly verbose
though. Maybe we can make it harder to misread things.

> And then the code neither actually tests that only a single bit is set,
> returning EINVAL if not,

It would be possible to add such a test, but doing so seems unwise:
there could be code out there that relies on the current, undefined
behavior, end we would end up breaking it.

> nor would its behaviour match the behaviour of acl_add_perm and 
> acl_delete_perm,
> both of which allow any combination of valid acl_perm_t bits.

This is undefined behavior that might differ in other implementations.

> Alternatively, if a bit combination is allowed as in acl_add_perm and
> acl_delete_perm, the test in the code is ok, but the evaluation of the
> return value is not (a&b!=0 vs. ~a&b==0).

It is undefined what acl_get_perm does when its perm argument is not
one of ACL_READ, ACL_WRITE, or ACL_EXECUTE. You are proposing a change
of this undefined behavior. However, this change could break code that
(wrongly) relies on the current behavior.

Thanks,
Andreas