[SSSD] [PATCH] enable debugging of krb5_child

Sumit Bose sbose at redhat.com
Thu Oct 15 12:32:26 UTC 2009


On Thu, Oct 15, 2009 at 06:59:09AM -0400, Stephen Gallagher wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 10/15/2009 05:18 AM, Sumit Bose wrote:
> > On Wed, Oct 14, 2009 at 01:58:36PM -0400, Stephen Gallagher wrote:
> >> -----BEGIN PGP SIGNED MESSAGE-----
> >> Hash: SHA1
> >>
> >> On 10/13/2009 03:52 AM, Sumit Bose wrote:
> >>> On Mon, Oct 12, 2009 at 10:28:05AM -0400, Simo Sorce wrote:
> >>>> On Mon, 2009-10-12 at 15:46 +0200, Sumit Bose wrote:
> >>>>> There is a problem with --debug-to-files. krb5_child runs as the user
> >>>>> requesting the ticket so the path to krb5_child.log needs to have
> >>>>> matching permissions. A possible solution would be to create the file
> >>>>> with 666 permissions during the setup of the kerberos backend. Any
> >>>>> other
> >>>>> ideas?
> >>>>
> >>>> You *really* don't want to have log files 666 ever.
> >>>> The easiest way would be to open the log file from the parent *without*
> >>>> CLOSE_ON_EXEC, and pass the fd number to krb5_child on the command line,
> >>>> and then have krb5_child use that fd to send debug messages.
> >>>>
> >>>> Simo.
> >>>>
> >>>
> >>> ok, please find updated patch attached.
> >>>
> >>> bye,
> >>> Sumit
> >>>
> >>>
> >>>
> >>> _______________________________________________
> >>> sssd-devel mailing list
> >>> sssd-devel at lists.fedorahosted.org
> >>> https://fedorahosted.org/mailman/listinfo/sssd-devel
> >>
> >> prepare_child_argv():
> >> Testing for argc < 2 for each of the potential options seems somewhat
> >> nonsensical, since you're starting at two (program name and NULL),
> >> adding one each for debug_level, debug_to_file and debug_timestamps and
> >> then subtracting them when you copy them in. I don't see anywhere that
> >> this check could ever fail to be true.
> > 
> > debug_level, debug_to_file and debug_timestamps are global variables, so
> > I thought it might be possible that in future they may change during an
> > interrupt.
> > 
> 
> Ok, first of all we don't use standard signal handlers in the SSSD. We
> use the tevent signal handlers which are very handy. When a signal is
> received, tevent notes it and adds an event to the mainloop that fires
> the next time the mainloop comes back around. This means that nothing
> will fire in the middle of other code execution.
> 
> Second, even if debug_level, debug_to_file or debug_timestamps changed
> during a signal handler, it wouldn't alter the contents of argc, since
> that's a local variable.
> 
> The absolute worst case here is that you might see debug_timestamps be
> set true when it should have been set false (if a signal changed things
> between 'if (debug_timestamps != 0)' and the talloc_strdup, but
> otherwise you will see it just copy in the integer value as it is at the
> time, so even if it changed, the effect would be the same.
> 
> For that matter, doing another check is no protection against this
> anyway, since an interrupt could still happen after the check.
> 
> I think the cleanest approach here is to just make the determination of
> what these will be at the top of the function, before the checks and
> just rely on those values. In other words, I think it is safe to go with:
> 
> static errno_t prepare_child_argv(TALLOC_CTX *mem_ctx,
>                                   struct krb5child_req *kr,
>                                   char ***_argv)
> {
>     uint_t argc = 3; /* program name, debug_level and NULL */
>     char ** argv;
>     errno_t ret = EINVAL;
> 
>     /* Save the current state in case an interrupt changes it */
>     bool child_debug_to_file = debug_to_file;
>     bool child_debug_timestamps = debug_timestamps;
> 
>     if (child_debug_to_file) argc++;
>     if (child_debug_timestamps) argc++;
> 
>     /* program name, debug_level,
>      * debug_to_file, debug_timestamps
>      * and NULL */
>     argv  = talloc_array(mem_ctx, char *, argc);
>     if (argv == NULL) {
>         DEBUG(1, ("talloc_array failed.\n"));
>         return ENOMEM;
>     }
> 
>     argv[--argc] = NULL;
> 
>     argv[--argc] = talloc_asprintf(argv, "--debug-level=%d",
>                               debug_level);
>     if (argv[argc] == NULL) {
>         ret = ENOMEM;
>         goto fail;
>     }
> 
>     if (child_debug_to_file) {
>         argv[--argc] = talloc_asprintf(argv, "--debug-fd=%d",
>                                   kr->krb5_ctx->child_debug_fd);
>         if (argv[argc] == NULL) {
>             ret = ENOMEM;
>             goto fail;
>         }
>     }
> 
>     if (child_debug_timestamps) {
>         argv[--argc] = talloc_strdup(argv, "--debug-timestamps");
>         if (argv[argc] == NULL) {
>             ret = ENOMEM;
>             goto fail;
>         }
>     }
> 
>     argv[--argc] = talloc_strdup(argv, KRB5_CHILD);
>     if (argv[argc] == NULL) {
>         ret = ENOMEM;
>         goto fail;
>     }
> 
>     if (argc != 0) {
>         ret = EINVAL;
>         goto fail;
>     }
> 
>     *_argv = argv;
> 
>     return EOK;
> 
> fail:
>     talloc_free(argv);
>     return ret;
> }
> 
> 
> >>
> >> Also, you don't test whether the talloc_strdup() calls might return NULL
> >> (in an out-of-memory situation).
> >>
> > 
> > ah, sorry, check are added. New patch attached
> > 
> >> The implementation looks fine otherwise.
> >>
> > 
> > Thanks.
> > 
> > bye,
> > Sumit
> > 
> > 
> > 
> > _______________________________________________
> > sssd-devel mailing list
> > sssd-devel at lists.fedorahosted.org
> > https://fedorahosted.org/mailman/listinfo/sssd-devel
> 
> I also noticed (and corrected in my example above) one other bug. In the
> failure case, you were doing a talloc_free(*argv); instead of
> talloc_free(argv).
> 

Thanks, it didn't came to my mind to safe the value in local variables.
ACK to your changes. Shall I send a new patch or will you integrate the
changes?

bye,
Sumit



More information about the sssd-devel mailing list