I was seeing issues with the samba 4 based libsmbclient library while using SMBC_setxattr() apis for setting the net security descriptor. Had a discussion on the samba-technical mailing list, where the problem is confirmed by Jeremy. Hence raising this issue. Putting the mail thread here for the quick reference : On Thu, Sep 03, 2015 at 02:22:07AM -0600, Har Gagan Sahai wrote: > > Hi Samba Team, > > > I have a question regarding the SMBC_setxattr() used to set the nt-security-descriptor. During the process of creating the request it is setting the 'ret = -1' which is causing the failure of the API. Here is the code snippet in cacl_set() [notice the statement in bold ] that I found to be causing the problem : > > > case SMBC_XATTR_MODE_ADD: > for (i=0;sd->dacl && i<sd->dacl->num_aces;i++) { > bool found = False; > > > for (j=0;old->dacl && j<old->dacl->num_aces;j++) { > if (dom_sid_equal(&sd->dacl->aces[i].trustee, > &old->dacl->aces[j].trustee)) { > if (!(flags & SMBC_XATTR_FLAG_CREATE)) { > err = EEXIST; > ret = -1; > goto failed; > } > old->dacl->aces[j] = sd->dacl->aces[i]; > ret = -1; > found = True; > } > } > > > if (!found && (flags & SMBC_XATTR_FLAG_REPLACE)) { > err = ENOATTR; > ret = -1; > goto failed; > } > > > for (i=0;sd->dacl && i<sd->dacl->num_aces;i++) { > add_ace(&old->dacl, &sd->dacl->aces[i], ctx); > } > } > dacl = old->dacl; > break; > > > Kindly let me know the reason behind this condition as this causing the failure of the usage. Incase this is not as intended let me know I can file a bug for this. Yes, I'm pretty sure the 'ret = -1' is a bug in this case. It should be: for (j=0;old->dacl && j<old->dacl->num_aces;j++) { if (dom_sid_equal(&sd->dacl->aces[i].trustee, &old->dacl->aces[j].trustee)) { if (!(flags & SMBC_XATTR_FLAG_CREATE)) { err = EEXIST; ret = -1; goto failed; } old->dacl->aces[j] = sd->dacl->aces[i]; found = True; } } However, I also think the check: if (!(flags & SMBC_XATTR_FLAG_CREATE)) { is incorrect. That flag means (from source3/include/libsmbclient.h): /* * Flags for smbc_setxattr() * Specify a bitwise OR of these, or 0 to add or replace as necessary */ #define SMBC_XATTR_FLAG_CREATE 0x1 /* fail if attr already exists */ #define SMBC_XATTR_FLAG_REPLACE 0x2 /* fail if attr does not exist */ so I think the correct code here should be: if (flags & SMBC_XATTR_FLAG_CREATE) { Also, the logic here of just overwriting the ace entry with the passed in one, and then adding it again in the code: for (i=0;sd->dacl && i<sd->dacl->num_aces;i++) { add_ace(&old->dacl, &sd->dacl->aces[i], ctx); } looks wrong again. That way we get 2 copies of the entry. Please log a bug and let's use that to coordinate working out what this code should really do. Jeremy.