[PATCH v2 01/12] Simplify allocations and fix merge and continue actions [BZ #28931]
DJ Delorie
dj@redhat.com
Wed Mar 16 20:47:06 GMT 2022
LGTM
Reviewed-by: DJ Delorie <dj@redhat.com>
Siddhesh Poyarekar via Libc-alpha <libc-alpha@sourceware.org> writes:
> diff --git a/nss/Makefile b/nss/Makefile
> +tests += tst-nss-gai-actions
New test OK.
> diff --git a/nss/tst-nss-gai-actions.c b/nss/tst-nss-gai-actions.c
> +enum
> +{
> + ACTION_MERGE = 0,
> + ACTION_CONTINUE,
> +};
> +
> +struct test_params
> +{
> + int action;
> + int family;
> + bool canon;
> +};
> +
> +struct support_chroot *chroot_env;
Ok.
> +static void
> +prepare (int argc, char **argv)
> +{
> + chroot_env = support_chroot_create
> + ((struct support_chroot_configuration)
> + {
> + .resolv_conf = "",
> + .hosts = "",
> + .host_conf = "multi on\n",
> + });
> +}
I'm curious why you chose to go this route rather than using the
test-container framework, which would have done most of this for you?
(but ok)
> +/* Create the /etc/hosts file from outside the chroot. */
> +static void
> +write_hosts (void)
> +{
> + const int count = 512;
> +
> + FILE *fp = xfopen (chroot_env->path_hosts, "w");
> + fputs ("127.0.0.1 localhost localhost.localdomain\n"
> + "::1 localhost localhost.localdomain\n",
> + fp);
> + for (int i = 1; i < count; ++i)
> + fprintf (fp, "192.0.%d.%d gnu.org\n", (i / 256) & 0xff, i & 0xff);
That's a lot of gnu.org :-)
> + xfclose (fp);
> +}
Ok.
> +static const char *
> +family_str (int family)
> +{
> + switch (family)
> + {
> + case AF_UNSPEC:
> + return "AF_UNSPEC";
> + case AF_INET:
> + return "AF_INET";
> + default:
> + __builtin_unreachable ();
> + }
> +}
Ok.
> +static const char *
> +action_str (int action)
> +{
> + switch (action)
> + {
> + case ACTION_MERGE:
> + return "merge";
> + case ACTION_CONTINUE:
> + return "continue";
> + default:
> + __builtin_unreachable ();
> + }
> +}
Ok.
> +/* getaddrinfo test. To be run from a subprocess. */
> +static void
> +test_gai (void *closure)
> +{
> + struct test_params *params = closure;
> +
> + struct addrinfo hints =
> + {
> + .ai_family = params->family,
> + };
> +
> + struct addrinfo *ai;
> +
> + if (params->canon)
> + hints.ai_flags = AI_CANONNAME;
> +
> + /* Use /etc/hosts in the chroot. */
> + xchroot (chroot_env->path_chroot);
> +
> + printf ("***** Testing \"files [SUCCESS=%s] files\" for family %s, %s\n",
> + action_str (params->action), family_str (params->family),
> + params->canon ? "AI_CANONNAME" : "");
> +
> + int ret = getaddrinfo ("gnu.org", "80", &hints, &ai);
> +
> + switch (params->action)
> + {
> + case ACTION_MERGE:
> + if (ret == 0)
> + {
> + char *formatted = support_format_addrinfo (ai, ret);
> +
> + printf ("merge unexpectedly succeeded:\n %s\n", formatted);
> + support_record_failure ();
> + free (formatted);
> + }
> + else
> + return;
> + case ACTION_CONTINUE:
> + {
> + char *formatted = support_format_addrinfo (ai, ret);
> +
> + /* Verify that the result appears exactly once. */
> + const char *expected = "address: STREAM/TCP 192.0.0.1 80\n"
> + "address: DGRAM/UDP 192.0.0.1 80\n"
> + "address: RAW/IP 192.0.0.1 80\n";
> +
> + const char *contains = strstr (formatted, expected);
> + const char *contains2 = NULL;
> +
> + if (contains != NULL)
> + contains2 = strstr (contains + strlen (expected), expected);
> +
> + if (contains == NULL || contains2 != NULL)
> + {
> + printf ("continue failed:\n%s\n", formatted);
> + support_record_failure ();
> + }
> +
> + free (formatted);
> + break;
> + }
> + default:
> + __builtin_unreachable ();
> + }
> +}
Ok.
> +static void
> +test_in_subprocess (int action)
> +{
> + char buf[32];
> +
> + snprintf (buf, sizeof (buf), "files [SUCCESS=%s] files",
> + action_str (action));
> + __nss_configure_lookup ("hosts", buf);
> +
> + struct test_params params =
> + {
> + .action = action,
> + .family = AF_UNSPEC,
> + .canon = false,
> + };
> + support_isolate_in_subprocess (test_gai, ¶ms);
> + params.family = AF_INET;
> + support_isolate_in_subprocess (test_gai, ¶ms);
> + params.canon = true;
> + support_isolate_in_subprocess (test_gai, ¶ms);
> +}
Ok.
> +static int
> +do_test (void)
> +{
> + support_become_root ();
> + if (!support_can_chroot ())
> + FAIL_UNSUPPORTED ("Cannot chroot\n");
> +
> + write_hosts ();
> + test_in_subprocess (ACTION_CONTINUE);
> + test_in_subprocess (ACTION_MERGE);
> +
> + support_chroot_free (chroot_env);
> + return 0;
> +}
Ok.
> +#define PREPARE prepare
> +#include <support/test-driver.c>
Ok.
> diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c
> index 18dccd5924..454a27eb2f 100644
> --- a/sysdeps/posix/getaddrinfo.c
> +++ b/sysdeps/posix/getaddrinfo.c
> @@ -458,11 +458,6 @@ gaih_inet (const char *name, const struct gaih_service *service,
>
> if (name != NULL)
> {
> - at = alloca_account (sizeof (struct gaih_addrtuple), alloca_used);
> - at->family = AF_UNSPEC;
> - at->scopeid = 0;
> - at->next = NULL;
> -
Moved below, ok.
> - if (__inet_aton_exact (name, (struct in_addr *) at->addr) != 0)
> + uint32_t addr[4];
> + if (__inet_aton_exact (name, (struct in_addr *) addr) != 0)
Use temp buffer because not allocated, ok.
> + at = alloca_account (sizeof (struct gaih_addrtuple), alloca_used);
> + at->scopeid = 0;
> + at->next = NULL;
> +
Ok.
> if (req->ai_family == AF_UNSPEC || req->ai_family == AF_INET)
> - at->family = AF_INET;
> + {
> + memcpy (at->addr, addr, sizeof (at->addr));
> + at->family = AF_INET;
> + }
Ok.
> else if (req->ai_family == AF_INET6 && (req->ai_flags & AI_V4MAPPED))
> {
> - at->addr[3] = at->addr[0];
> + at->addr[3] = addr[0];
Ok.
> @@ -493,49 +496,62 @@ gaih_inet (const char *name, const struct gaih_service *service,
>
> if (req->ai_flags & AI_CANONNAME)
> canon = name;
> +
> + goto process_list;
ok.
> }
> - else if (at->family == AF_UNSPEC)
> +
> + char *scope_delim = strchr (name, SCOPE_DELIMITER);
> + int e;
> +
> + if (scope_delim == NULL)
> + e = inet_pton (AF_INET6, name, addr);
> + else
> + e = __inet_pton_length (AF_INET6, name, scope_delim - name, addr);
Moved out of scope, ok.
> +
> + if (e > 0)
Moved up, ok.
> {
> - char *scope_delim = strchr (name, SCOPE_DELIMITER);
> - int e;
> - if (scope_delim == NULL)
> - e = inet_pton (AF_INET6, name, at->addr);
Move up, ok.
> + at = alloca_account (sizeof (struct gaih_addrtuple),
> + alloca_used);
> + at->scopeid = 0;
> + at->next = NULL;
> +
> + if (req->ai_family == AF_UNSPEC || req->ai_family == AF_INET6)
> + {
> + memcpy (at->addr, addr, sizeof (at->addr));
> + at->family = AF_INET6;
> + }
> + else if (req->ai_family == AF_INET
> + && IN6_IS_ADDR_V4MAPPED (addr))
> + {
> + at->addr[0] = addr[3];
> + at->addr[1] = addr[1];
> + at->addr[2] = addr[2];
> + at->addr[3] = addr[3];
> + at->family = AF_INET;
> + }
Moved, ok.
> else
> - e = __inet_pton_length (AF_INET6, name, scope_delim - name,
> - at->addr);
> - if (e > 0)
> {
> - if (req->ai_family == AF_UNSPEC || req->ai_family == AF_INET6)
> - at->family = AF_INET6;
> - else if (req->ai_family == AF_INET
> - && IN6_IS_ADDR_V4MAPPED (at->addr))
> - {
> - at->addr[0] = at->addr[3];
> - at->family = AF_INET;
> - }
> - else
> - {
> - result = -EAI_ADDRFAMILY;
> - goto free_and_return;
> - }
> -
> - if (scope_delim != NULL
> - && __inet6_scopeid_pton ((struct in6_addr *) at->addr,
> - scope_delim + 1,
> - &at->scopeid) != 0)
> - {
> - result = -EAI_NONAME;
> - goto free_and_return;
> - }
Moved, ok.
> + result = -EAI_ADDRFAMILY;
> + goto free_and_return;
> + }
Moved, ok.
> - if (req->ai_flags & AI_CANONNAME)
> - canon = name;
> + if (scope_delim != NULL
> + && __inet6_scopeid_pton ((struct in6_addr *) at->addr,
> + scope_delim + 1,
> + &at->scopeid) != 0)
> + {
> + result = -EAI_NONAME;
> + goto free_and_return;
Moved, ok.
> }
> +
> + if (req->ai_flags & AI_CANONNAME)
> + canon = name;
> +
> + goto process_list;
> }
Ok.
> - if (at->family == AF_UNSPEC && (req->ai_flags & AI_NUMERICHOST) == 0)
> + if ((req->ai_flags & AI_NUMERICHOST) == 0)
Ok.
> {
> - struct gaih_addrtuple **pat = &at;
Ok.
> + bool do_merge = false;
Ok.
> @@ -579,7 +596,7 @@ gaih_inet (const char *name, const struct gaih_service *service,
> result = -EAI_MEMORY;
> goto free_and_return;
> }
> - *pat = addrmem;
> + at = addrmem;
Ok.
> struct gaih_addrtuple *addrfree = addrmem;
> + struct gaih_addrtuple **pat = &at;
> +
Ok.
> free (air);
>
> - if (at->family == AF_UNSPEC)
> - {
> - result = -EAI_NONAME;
> - goto free_and_return;
> - }
> -
Ok.
> @@ -732,6 +745,21 @@ gaih_inet (const char *name, const struct gaih_service *service,
>
> while (!no_more)
> {
> + /* Always start afresh; continue should discard previous results
> + and the hosts database does not support merge. */
> + at = NULL;
> + free (canonbuf);
> + free (addrmem);
> + canon = canonbuf = NULL;
> + addrmem = NULL;
> +
> + if (do_merge)
> + {
> + __set_h_errno (NETDB_INTERNAL);
> + __set_errno (EBUSY);
> + break;
> + }
> +
Ok.
> - status = DL_CALL_FCT (fct4, (name, pat,
> + status = DL_CALL_FCT (fct4, (name, &at,
Ok.
> + /* gethostbyname4_r may write into AT, so reset it. */
> + at = NULL;
This is alloca-ted, so no leak here. ok.
> @@ -774,7 +804,9 @@ gaih_inet (const char *name, const struct gaih_service *service,
> no_data = 1;
>
> if ((req->ai_flags & AI_CANONNAME) != 0 && canon == NULL)
> - canon = (*pat)->name;
> + canon = at->name;
> +
> + struct gaih_addrtuple **pat = &at;
Ok.
> while (*pat != NULL)
> {
> @@ -826,6 +858,8 @@ gaih_inet (const char *name, const struct gaih_service *service,
>
> if (fct != NULL)
> {
> + struct gaih_addrtuple **pat = &at;
> +
Ok.
> + /* The hosts database does not support MERGE. */
> + if (nss_next_action (nip, status) == NSS_ACTION_MERGE)
> + do_merge = true;
> +
Ok.
> process_list:
> - if (at->family == AF_UNSPEC)
> + if (at == NULL)
Ok.
More information about the Libc-alpha
mailing list