-----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