Question on SMBC_setxattr() in libsmbclient library
Jeremy Allison
jra at samba.org
Thu Sep 3 23:50:31 UTC 2015
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.
More information about the samba-technical
mailing list