[SSSD] [PATCH] sudo: provide automatic configuration of machine hostnames

Pavel Březina pbrezina at redhat.com
Tue Jul 24 10:21:20 UTC 2012


On 23.7.2012 19:00, Jakub Hrozek wrote:
> On Mon, Jul 23, 2012 at 01:09:13PM +0200, Jan Zelený wrote:
>> Dne pondělí 23 července 2012 09:46:07, Pavel Březina napsal(a):
>>> On 07/20/2012 10:47 AM, Jakub Hrozek wrote:
>>>> On Thu, Jul 19, 2012 at 07:28:18PM +0200, Pavel Březina wrote:
>>>>> On 19.7.2012 13:18, Jan Zelený wrote:
>>>>>> Dne pondělí 16 července 2012 16:01:46, Pavel Březina napsal(a):
>>>>>>> Expects that patch from "resolv_gethostbyname_send: talloc_strdup
>>>>>>> hostname on state" thread is applied.
>>>>>>
>>>>>> Nack,
>>>>>> please don't use talloc_realloc () in sdap_sudo_get_hostnames_send(),
>>>>>> it's
>>>>>> confusing. Allocating an array for three elements with
>>>>>> talloc_zero_array()
>>>>>> will save you couple lines of code (all those reallocs and NULL
>>>>>> assignments).
>>>>>>
>>>>>> This is of course based on two observations I've made in the code are
>>>>>> that
>>>>>> before allocation:
>>>>>> - the state->hostnames is NULL when calling
>>>>>> sdap_sudo_get_hostnames_send()
>>>>>> - in there is no error, the state->hostnames will contain two records
>>>>>> and NULL
>>>>>>
>>>>>> You are not checking if talloc_strdup() calls returned non-NULL pointer.
>>>>>
>>>>> Thanks. New patch is attached.
>>>>>
>>>>>> I suspect you should use CONFDB_DOMAIN_RESOLV_OP_TIMEOUT instead of
>>>>>> CONFDB_DOMAIN_RESOLV_TIMEOUT. I'm not sure though, please take a look at
>>>>>> data_provider_fo.c:106 and provide a comment.
>>>>>
>>>>> Well, I don't know. What's the difference between those two? I was
>>>>> following ipa_dyndns.c and the shorter version is used there (see line
>>>>> 148).
>>>>
>>>> CONFDB_DOMAIN_RESOLV_OP_TIMEOUT (aka dns_resolver_op_timeout) is a very
>>>> low-level setting that fine-tunes how long should c-ares talk to
>>>> individual
>>>> nameservers before giving up and trying the next DNS server. It is not
>>>> documented and left out of configAPI.
>>>>
>>>> CONFDB_DOMAIN_RESOLV_TIMEOUT (aka dns_resolver_timeout) is a fail over
>>>> setting that tunes how long should a service resolution take. Keep in mind
>>>> that "service resolution" might involve talking to different DNS servers.
>>>>
>>>> Unless you want to control timeouts related to a single DNS server,
>>>> CONFDB_DOMAIN_RESOLV_TIMEOUT is what you'd want to use.
>>>>
>>>> https://fedorahosted.org/sssd/ticket/976 has more details.
>>>
>>> Thank you for the explanation. Than I believe it is OK to use
>>> CONFDB_DOMAIN_RESOLV_TIMEOUT in this case.
>>
>> I agree, Ack
>>
>> Jan
>
> Nack,
>
> +#include <unistd.h>
> +#include <bits/local_lim.h> // HOST_NAME_MAX
> +#include <string.h>
>
> Please include only <limits.h> here, HOST_NAME_MAX will be included from
> local_lim.h (via another header called posix_lim.h or similarly..)

Done.

> Also don't use C++ comments.

Removed.

> +    char hostname[HOST_NAME_MAX + 1];
> +    errno = 0;
> +    ret = gethostname(hostname, sizeof(hostname));
> +    if (ret != EOK) {
>
>
> This looks like a potential buffer overrun to me for cases when the host
> name is HOST_NAME_MAX+1 long, I think you should use HOST_NAME_MAX as
> the size parameter and also unconditionally set hostname[HOST_NAME_MAX]
> to '\0' to make sure the buffer is NULL-terminated.

Done.

> The rest looks good, I'm just wondering that maybe it would be nice to
> create a utility function to initialize resolver with the defaults that
> could be reused in data_provider_fo.c and this code.

+1
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-sudo-ldap-provider-support-autoconfiguration-of-host.patch
Type: text/x-patch
Size: 10518 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120724/faa10c36/attachment.bin>


More information about the sssd-devel mailing list