[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