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

[Pavel Reichl]

Hello,

please see attached simple patch.

Please note that this patch depends on patch from thread:

RESPONDERS: refactor create_pipe_fd()

Thanks!

[Jakub Hrozek]

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)

[Michal Židek]

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.

Michal

[Jakub Hrozek]

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?

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.

[Michal Židek]

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.

[Pavel Reichl]

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(a)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.

[Jakub Hrozek]

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(a)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(a)redhat.com&gt;
>  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(a)redhat.com&gt;
>  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..

[Jakub Hrozek]

On Tue, Oct 28, 2014 at 05:50:19AM +0100, Jakub Hrozek wrote:
>  > From 39277d9408527dd364baba1effb76c4d87789ee7 Mon Sep 17
> 00:00:00 2001
>  > From: Pavel Reichl <preichl(a)redhat.com&gt;
>  > 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(a)redhat.com&gt;
>  > 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.. 
* master:
    cbcb834028794a4c658a85965516113f8c0760c1
    458f5245dd5130d12666cce6faf8ef1ec7f80169