[PATCH] vfs_glusterfs: Make sure posix acls are formatted "before setting"/"after getting"

[Simo]

On Fri, 2013-08-09 at 17:37 -0700, Jeremy Allison wrote:
> On Fri, Aug 09, 2013 at 05:50:46PM -0500, Christopher R. Hertel wrote:
> > I've updated the patch (fixed the formatting to meet Samba standards).
> > +1 for the attached.
> > 
> > Need one more approval so that I can commit.
> 
> I'm ok with this specific change, except for
> the change you made to do:
> 
> +static int gluster_ace_cmp (const void *val1, const void *val2)
> +{
> +       const struct gluster_ace *ace1 = NULL;
> +       const struct gluster_ace *ace2 = NULL;
> +       int                        ret = 0;
> 
> Instead of the original:
> 
> +static int gluster_ace_cmp (const void *val1, const void *val2)
> +{
> +       const struct gluster_ace *ace1 = NULL;
> +       const struct gluster_ace *ace2 = NULL;
> +       int ret = 0;
> 
> Please put that back :-). Your formatting there
> makes this completely unreadable.
> 
> However in reviewing this patch I noticed
> something else that's really wrong with
> this module. I probably should have caught
> it before if I was the reviewer of the code,
> so it might be my fault.
> 
> The linearization of the gluster ACLs is
> completely wrong for portable code.
> 
> Look at this code:
> 
> -------------------------------------------------
> struct gluster_ace {
>         uint16_t tag;
>         uint16_t perm;
>         uint32_t id;
> };
> 
> struct gluster_acl_header {
>         uint32_t version;
>         struct gluster_ace entries[];
> };
> 
> .....
> 
> static ssize_t smb_to_gluster_acl(SMB_ACL_T theacl, char *buf, size_t len)
> {
>         ssize_t size;
>         struct gluster_ace *ace;
>         struct smb_acl_entry *smb_ace;
>         struct gluster_acl_header *hdr;
>         int i;
>         int count;
>         uint16_t tag;
>         uint16_t perm;
>         uint32_t id;
> 
>         count = theacl->count;
> 
>         size = sizeof(*hdr) + (count * sizeof(*ace));
>         if (!buf) {
>                 return size;
>         }
> 
>         if (len < size) {
>                 errno = ERANGE;
>                 return -1;
>         }
> 
>         hdr = (void *)buf;
> 
> -------------------------------------------------
> 
> It's taking a passed in buffer of a given length, then casting a
> pointer to a structure to point at that buffer as though
> it can then write into it as if it were actually a structure
> of that type.
> 
> It's not doing proper marshalling/unmarshalling at all !
> 
> You can't just plonk a structure into unstructured memory,
> store it on disk and assume it's going to be read back
> the same way from another machine compiled with a different
> compiler on a different CPU type (which I'm assuming a
> gluster cluster can easily consist of).
> 
> The functions smb_to_gluster_acl()/gluster_to_smb_acl()
> need to be completely rewritten to correctly marshall/unmarshall
> every element in these structure definitions.
> 
> We've had enough bugs around this in the past to know
> that this simply will not work for portable code. It
> might be working on systems compiled with the same
> compiler, but there's no guarentee that structure padding
> won't change when compilers get updated, or if a
> different compiler is used.

You can use pragma directives to make sure the structure is always fully
packed.

#pragma pack(1)
struct gluster_ace {
    uint16_t tag;
    uint16_t perm;
    uint32_t id;
};

struct gluster_acl_header {
    uint32_t version;
    struct gluster_ace entries[];
};
#pragma pack()

Given there are no pointers in these structures and the type of the
members has the same size on any (sane) architecture, it should work
just fine.

The only problem is that you still need to care for bigendian vs little
endian when storing the 3 values.

Simo.

> I'm CC:ing Anand Avati on this reply to ensure this
> gets redone right.
> 
> This needs addressing ASAP. You can't ship code
> with this in.
> 
> Anand, can you get this fixed up please ?
> 
> Thanks,
> 
> Jeremy.