-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 10/15/2009 08:32 AM, Sumit Bose wrote: > 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@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@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 > _______________________________________________ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://fedorahosted.org/mailman/listinfo/sssd-devel
Send a new patch, please. You have the infrastructure in place to test it better than I can right now. - -- Stephen Gallagher RHCE 804006346421761 Looking to carve out IT costs? www.redhat.com/carveoutcosts/ -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/ iEYEARECAAYFAkrXFsgACgkQeiVVYja6o6OMkgCgplcehDvSRigS20qt+T/wc/a/ sgUAn1dLKwic3qt1UeE1bObV8HRTiHRk =hDoF -----END PGP SIGNATURE----- _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
