[SSSD] [PATCH] enable debugging of krb5_child

Stephen Gallagher sgallagh at redhat.com
Thu Oct 15 12:34:22 UTC 2009


-----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 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
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at 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-----



More information about the sssd-devel mailing list