-----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(a)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(a)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(a)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?
-----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-----