What's wrong with this piece of code from source3/modules/vfs_acl_xattr.c around like 126 or so?

[Richard Sharpe]

On Tue, May 15, 2012 at 6:45 AM, Richard Sharpe
<realrichardsharpe at gmail.com> wrote:
> On Mon, May 14, 2012 at 10:58 PM, Michael Adam <obnox at samba.org> wrote:
>> Hi Richard,
>>
>> I guess the point that the code is trying to make is that
>> it wants to protect the original errno from being overwritten
>> by intermediate actions like unbecome_root, debugging, etc.
>> unbecome_root does quite a lot of operations..
>>
>> To do that consequently we could change that according
>> to the attached patch. Does that make it more clear
>> and maybe more correct? With this patch, the errno is only
>> reset to the saved value directly before the return.
>
> That's very funny:
>
>        }
>        unbecome_root();
>        if (ret) {
> -               errno = saved_errno;
>                DEBUG(5, ("store_acl_blob_fsp: setting attr failed for file %s"
>                        "with error %s\n",
>                        fsp_str_dbg(fsp),
> -                       strerror(errno) ));
> -               return map_nt_error_from_unix(errno);
> +                       strerror(saved_errno) ));
> +               return map_nt_error_from_unix(saved_errno);
> +               errno = saved_errno;
>        }
>        return NT_STATUS_OK;
>  }
>
> I suspect that a good compiler would say that the statement after the
> return has no effect :-)
>
> The intent is close. There is an error in the caller as well. It
> discards the return code. I will fix the problem in master and commit
> it.

These are the changes I am thinking of. They are building now and if
they build cleanly, I will commit them later today (after further
tests at work.)

diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c
index 241bc8f..221b43f 100644
--- a/source3/modules/vfs_acl_common.c
+++ b/source3/modules/vfs_acl_common.c
@@ -592,10 +592,13 @@ static NTSTATUS fset_nt_acl_common(vfs_handle_struct *hand
                NDR_PRINT_DEBUG(security_descriptor,
                        discard_const_p(struct security_descriptor, psd));
        }
+       /*
+        * Perhaps create_acl_blob should have a status return as well
+        */
        create_acl_blob(psd, &blob, XATTR_SD_HASH_TYPE_SHA256, hash);
-       store_acl_blob_fsp(handle, fsp, &blob);
+       status = store_acl_blob_fsp(handle, fsp, &blob);

-       return NT_STATUS_OK;
+       return status;
 }

 static int acl_common_remove_object(vfs_handle_struct *handle,
diff --git a/source3/modules/vfs_acl_xattr.c b/source3/modules/vfs_acl_xattr.c
index 5653657..17d1a8f 100644
--- a/source3/modules/vfs_acl_xattr.c
+++ b/source3/modules/vfs_acl_xattr.c
@@ -119,12 +119,11 @@ static NTSTATUS store_acl_blob_fsp(vfs_handle_struct *hand
        }
        unbecome_root();
        if (ret) {
-               errno = saved_errno;
                DEBUG(5, ("store_acl_blob_fsp: setting attr failed for file %s"
                        "with error %s\n",
                        fsp_str_dbg(fsp),
-                       strerror(errno) ));
-               return map_nt_error_from_unix(errno);
+                       strerror(saved_errno) ));
+               return map_nt_error_from_unix(saved_errno);
        }
        return NT_STATUS_OK;
 }


-- 
Regards,
Richard Sharpe
(何以解憂？唯有杜康。--曹操)