[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