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

[Jeremy Allison]

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.

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.