8:46 a.m.
Hi,
Although there are lots of DEBUG calls in krb5_child it always runs with
debug_level=0. This patch starts krb5_child with the debugging options
of the backend.
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?
bye,
Sumit
8:49 a.m.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 10/12/2009 09:46 AM, Sumit Bose wrote:
Hi,
Although there are lots of DEBUG calls in krb5_child it always runs with
debug_level=0. This patch starts krb5_child with the debugging options
of the backend.
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?
This is what the ELAPI is for. We'll take advantage of it when it's
ready. In the meantime, I think it's probably ok to ignore this option
in the child, for security reasons. We don't want other users being able
to read the authentication logs for a user!
bye,
Sumit
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
- --
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/
iEYEARECAAYFAkrTM90ACgkQeiVVYja6o6OHpwCfSUqB/REtB06083Wlt0Tlr7TV
a0IAn14JXBlNMZ6o1BRpKO3Nwt9ZujfL
=fkiI
-----END PGP SIGNATURE-----
9:29 a.m.
On Mon, 2009-10-12 at 09:49 -0400, Stephen Gallagher wrote:
This is what the ELAPI is for.
No ELAPI is for logging not for the debug stuff, and an api can't
overcome file permissions no more than anything else.
Just pass the fd to the client, it's simple and doesn't require us to
replicate logic to open/close debug files in the children.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
9:35 a.m.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 10/12/2009 10:29 AM, Simo Sorce wrote:
On Mon, 2009-10-12 at 09:49 -0400, Stephen Gallagher wrote:
> This is what the ELAPI is for.
No ELAPI is for logging not for the debug stuff, and an api can't
overcome file permissions no more than anything else.
ELAPI has sinks that could pass it to the log daemon running as root.
This is what I was proposing.
Just pass the fd to the client, it's simple and doesn't require us to
replicate logic to open/close debug files in the children.
I didn't realize you could do that.
Simo.
- --
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/
iEYEARECAAYFAkrTPrwACgkQeiVVYja6o6PVjQCggt/9ZM3hRAavrkM9EZq/Ccks
IskAn3GJGEoZh22u6ebVSXrNVy8YjaUs
=v0ku
-----END PGP SIGNATURE-----
9:47 a.m.
Stephen Gallagher wrote:
On 10/12/2009 10:29 AM, Simo Sorce wrote:
> On Mon, 2009-10-12 at 09:49 -0400, Stephen Gallagher wrote:
>> This is what the ELAPI is for.
> No ELAPI is for logging not for the debug stuff, and an api can't
> overcome file permissions no more than anything else.
ELAPI has sinks that could pass it to the log daemon running as root.
This is what I was proposing.
ELAPI is intended to be used for debug streams too.
One of the things I was planning to add is permissions and ownership of
the log file.
I will open a ticket for that anyways.
> Just pass the fd to the client, it's simple and doesn't require us to
> replicate logic to open/close debug files in the children.
I didn't realize you could do that.
I am not sure this approach is portable.
I know Solaris and Linux can do it.
I am not sure HP-UX can.
> Simo.
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
--
Thank you,
Dmitri Pal
Engineering Manager IPA project,
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
10:58 a.m.
On Mon, 2009-10-12 at 10:47 -0400, Dmitri Pal wrote:
> > Just pass the fd to the client, it's simple and
doesn't require us to
> > replicate logic to open/close debug files in the children.
>
> I didn't realize you could do that.
>
I am not sure this approach is portable.
I know Solaris and Linux can do it.
I am not sure HP-UX can.
We are not *transferring* a socket between process, we are merely not
closing it on fork/exec. It is standard posix behavior that file
descriptors are inherited by children afaik.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
11:10 a.m.
Simo Sorce wrote:
On Mon, 2009-10-12 at 10:47 -0400, Dmitri Pal wrote:
>>> Just pass the fd to the client, it's simple and doesn't require us
to
>>> replicate logic to open/close debug files in the children.
>>>
>> I didn't realize you could do that.
>>
>>
> I am not sure this approach is portable.
> I know Solaris and Linux can do it.
> I am not sure HP-UX can.
>
We are not *transferring* a socket between process, we are merely not
closing it on fork/exec. It is standard posix behavior that file
descriptors are inherited by children afaik.
Simo.
Yes this way it is standard. I thought you wanted to pass a socket
between processes later after fork.
But here is the question. If you have multiple children writing to the
same fd at the same time how you then sort which one has written what.
Would it be better to have a log per child process instead and have a
pid appended to the name of the log file than all output in one file mixed?
It is usually hard to read and debug when everything is mixed in one file.
--
Thank you,
Dmitri Pal
Engineering Manager IPA project,
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
11:48 a.m.
On Mon, 2009-10-12 at 12:10 -0400, Dmitri Pal wrote:
Yes this way it is standard. I thought you wanted to pass a socket
between processes later after fork.
But here is the question. If you have multiple children writing to the
same fd at the same time how you then sort which one has written what.
Would it be better to have a log per child process instead and have a
pid appended to the name of the log file than all output in one file
mixed?
It is usually hard to read and debug when everything is mixed in one
file.
This is used only for debugging, we can stand some mixed inputs there to
keep the thing simple, however in the children we can change the debug
function to always print the PID on every debug message, so that at
least it should be clear where messages came from.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
11:55 a.m.
Simo Sorce wrote:
On Mon, 2009-10-12 at 12:10 -0400, Dmitri Pal wrote:
> Yes this way it is standard. I thought you wanted to pass a socket
> between processes later after fork.
> But here is the question. If you have multiple children writing to the
> same fd at the same time how you then sort which one has written what.
> Would it be better to have a log per child process instead and have a
> pid appended to the name of the log file than all output in one file
> mixed?
> It is usually hard to read and debug when everything is mixed in one
> file.
>
This is used only for debugging, we can stand some mixed inputs there to
keep the thing simple, however in the children we can change the debug
function to always print the PID on every debug message, so that at
least it should be clear where messages came from.
Simo.
Yes we can add pids to every line but from my experience such logs are
very hard to read when debugging a program.
You have a mixture of relevant and irrelevant information in one
sequence of lines. IMO it is very annoying and hard to process.
--
Thank you,
Dmitri Pal
Engineering Manager IPA project,
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
12:40 p.m.
On Mon, Oct 12, 2009 at 12:10:43PM -0400, Dmitri Pal wrote:
Simo Sorce wrote:
> On Mon, 2009-10-12 at 10:47 -0400, Dmitri Pal wrote:
>
>
>>>> Just pass the fd to the client, it's simple and doesn't require
us to
>>>> replicate logic to open/close debug files in the children.
>>>>
>>> I didn't realize you could do that.
>>>
>>>
>> I am not sure this approach is portable.
>> I know Solaris and Linux can do it.
>> I am not sure HP-UX can.
>>
>
> We are not *transferring* a socket between process, we are merely not
> closing it on fork/exec. It is standard posix behavior that file
> descriptors are inherited by children afaik.
>
> Simo.
>
>
Yes this way it is standard. I thought you wanted to pass a socket
between processes later after fork.
But here is the question. If you have multiple children writing to the
same fd at the same time how you then sort which one has written what.
Would it be better to have a log per child process instead and have a
pid appended to the name of the log file than all output in one file mixed?
It is usually hard to read and debug when everything is mixed in one file.
--
Thank you,
Dmitri Pal
Currently I'll use the pid in the starting block of the log messsage.
bye,
Sumit
9:28 a.m.
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.
--
Simo Sorce * Red Hat, Inc * New York
2:52 a.m.
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
12:58 p.m.
-----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.
Also, you don't test whether the talloc_strdup() calls might return NULL
(in an out-of-memory situation).
The implementation looks fine otherwise.
- --
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/
iEYEARECAAYFAkrWEUcACgkQeiVVYja6o6MdcQCeMDPK85b1csjASsVmOWWUyjOV
VZAAoKvcAFDggdSarb7j61R0U5DDnpcI
=e45z
-----END PGP SIGNATURE-----
4:18 a.m.
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.
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
5:59 a.m.
-----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).
- --
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/
iEYEARECAAYFAkrXAHIACgkQeiVVYja6o6O1vwCgpP9BAxOQkWpfqGFMGvTHRr3r
HKwAnRarv2AXff38sE27ZL6VK5NSKcoa
=9/su
-----END PGP SIGNATURE-----
7:32 a.m.
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
7:34 a.m.
-----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?
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-----
5300
days inactive
5303
days old
sssd-devel@lists.fedorahosted.org
18 comments
4 participants
participants (4)
-
Dmitri Pal -
Simo Sorce -
Stephen Gallagher -
Sumit Bose