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 > >>> [email protected] > >>> 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 > > [email protected] > > 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 _______________________________________________ sssd-devel mailing list [email protected] https://fedorahosted.org/mailman/listinfo/sssd-devel
