[SSSD] [PATCH] RESPONDERS: Don't set umask in utility function

Jakub Hrozek jhrozek at redhat.com
Tue Oct 28 04:50:19 UTC 2014


On Fri, Oct 24, 2014 at 04:11:12PM +0200, Pavel Reichl wrote:
> 
> On 10/23/2014 07:25 PM, Michal Židek wrote:
> >On 10/23/2014 07:17 PM, Jakub Hrozek wrote:
> >>On Thu, Oct 23, 2014 at 07:12:44PM +0200, Michal Židek wrote:
> >>>On 10/23/2014 07:02 PM, Jakub Hrozek wrote:
> >>>>On Thu, Oct 23, 2014 at 05:30:56PM +0200, Pavel Reichl wrote:
> >>>>>-    ret = create_pipe_fd(ctx->sock_name, &ctx->fd, 0111);
> >>>>>+    umask(0111);
> >>>>>+    ret = create_pipe_fd(ctx->sock_name, &ctx->fd);
> >>>>>+    umask(DEF_RSP_UMASK);
> >>>>>      assert_int_equal(ret, EOK);
> >>>>>      assert_int_not_equal(ctx->fd, -1);
> >>>>>      check_sock_properties(ctx, 0666);
> >>>>>
> >>>>>      /* Make sure we can overwrite an existing socket */
> >>>>>-    ret = create_pipe_fd(ctx->sock_name, &ctx->fd, 0000);
> >>>>>+    umask(0000);
> >>>>>+    ret = create_pipe_fd(ctx->sock_name, &ctx->fd);
> >>>>>+    umask(DEF_RSP_UMASK);
> >>>>
> >>>>I don't like a pattern like this that requires the caller to set and
> >>>>reset the caller. I would prefer if we still passed either the
> >>>>required
> >>>>mode or umask to the create_pipe_fd(), then create_pipe_fd() would
> >>>>first
> >>>>save the old umask, set the new one, create the pipes and then reset
> >>>>the
> >>>>old one in the "done" handler.
> >>>>
> >>>>The default umask should be set in a very generic function like
> >>>>server_setup (maybe we already do that, not sure)
> >>>
> >>>It can not be done in server_setup (it is too late). So we can
> >>>have a new "setup" like function or we just call umask after
> >>>the process is created (in main) with some #defined value.
> >>
> >>If the new function called just umask then I don't see a reason for the
> >>wrapper -- maybe for future extensibility?
> >
> >I think calling umask will be just fine. No new function needed,
> >I should not have mentioned it probably.
> >
> >>
> >>But I'm fine with both these options, I just don't like us setting an
> >>umask
> >>in create_pipe_fd without resetting in back in the same function. A
> >>utility function should have no side effects.
> >
> >
> >_______________________________________________
> >sssd-devel mailing list
> >sssd-devel at lists.fedorahosted.org
> >https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> OK, please see updated version.
> 
> I was wondering if we should create some function like
> responder_common_init() which would be called in each responder in the
> beginning of main() to provide common functionality like setting umask.
> 
> Thanks.

> From 39277d9408527dd364baba1effb76c4d87789ee7 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl <preichl at redhat.com>
> Date: Fri, 24 Oct 2014 10:06:55 +0100
> Subject: [PATCH 1/2] RESPONDERS: Don't hard-code umask value in utility

ACK


> From eb11d17b94f217f7dbfda5f264d46585cd8fbef2 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl <preichl at redhat.com>
> Date: Fri, 24 Oct 2014 12:42:50 +0100
> Subject: [PATCH 2/2] RESPONDERS: Set default value for umask
> 
> Resolves:
> https://fedorahosted.org/sssd/ticket/

ACK

Although we don't strictly need to set restrictive umask in other
responder than PAM, it's not harmful either and might actually come
handy when/if we add more content..



More information about the sssd-devel mailing list