[PATCHES v2] Fix FreeBSD developer build
Uri Simchoni
uri at samba.org
Wed Nov 22 06:02:44 UTC 2017
On 11/22/2017 03:18 AM, Andrew Bartlett wrote:
> One change that I've reviewed, but I'm not very happy with is this one:
>
> sysacls: make SMB_ACL_PERMSET_T opaque
>
> sys_acl_get_permset() returns a pointer to a uint32_t. Prior
> to this change, SMB_ACL_PERMSET_T has been defined as a mode_t
> pointer, where mode_t may or may not be uint32_t (on FreeBSD
> it's uint16_t, which could screw up big-endian FreeBSD systems).
>
> Since SMB_ACL_PERMSET_T is being used as an opaque
> data type to the ouside world, this patch changes it
> to a void*, and in the implementation casts the pointer
> to a uint32_t pointer.
>
> Signed-off-by: Uri Simchoni <uri at samba.org>
> Reviewed-by: Andrew Bartlett <abartlet at samba.org>
>
> Could you see if you could re-do it to instead change the interface
> where the mode_t matters, and fix it as a uint32_t in the type. void*
> is to be avoided if possible.
>
It does seem like this whole thing can be greatly simplified, I can have
a go at it in a few days. Perhaps this whole PERMSET_T business is a
reminiscent of an era before POSIX ACLs were done via the VFS, and some
opaqueness was necessary to shield the code from os-specific parts. It
looks, from first glance, that I can totally remove it.
> Can you also explain more how the change in the type hurts things?
>
The code casts a uint32_t pointer to a mode_t pointer. On Linux, both
are 32 bits - doesn't matter, but on FreeBSD mode_t is 16 bits and
things get interesting.
Internally, the permissions in each ACL entry are stored in a uint32_t.
The VFS is responsible for fetching the entry through some means,
without using this code. The user-group-other bits occupy the least
significant bits. On little-endian systems, those reside on the first
byte, and if we interpret that memory location as storing a 16-bit
value, we still get those bits as the least-significant bits. However,
on big-endian systems the mode bits reside on the 4th byte, and when
interpreting the memory location as 16-bits, we get the most-significant
16 bits of the original 32-bit quantity - probably zeros.
That actually reminds me of this thread concerning hpux and ia64:
https://lists.samba.org/archive/samba-technical/2016-November/117232.html
If you read it through you'll see I was unable to find the issue because
I was looking at the VFS layer, whereas the the problem might have been
in the os-independent code.
A bit of googling suggests that hpux uses big-endian on ia64 (from hw
perspective it's selectable)
To summarize:
- Linux - no problem, mode_t is 32 bits
- Little-endian - no problem, 16-bit is exchangeable with 32-bits
- Big-endian and mode_t is 16 bits - probably a problem.
> If it can't be done, then my review stands, but can you take a look
> again?
>
I will, and it can be done, but it might take a few days.
> In the meantime I've pushed the rest of this partial set to autobuild,
> hopefully that makes review of the remainder easier.
>
Thanks for taking the time to reviewing this.
> Thanks,
>
> Andrew Bartlett
>
More information about the samba-technical
mailing list