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

Jakub Hrozek jhrozek at redhat.com
Mon Jul 23 17:00:55 UTC 2012


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

Also don't use C++ comments.

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

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.



More information about the sssd-devel mailing list