On Mon, 2014-04-21 at 23:15 +0200, Jakub Hrozek wrote:
> Hi,
>
> the attached patches implement ticket #2073 -- the possibility to
> extend
> the LDAP attribute map with custom attributes.
>
> All attributes are saved to sysdb with the 'extra_' prefix so that the
> custom attributes can be distinguished from the sysdb schema.
>
> The first patch fixes a mostly unrelated bug, which I found when
> writing
> unit tests.
>
> [PATCH 1/3] LDAP: Fix one-by-one bug in sdap_copy_opts
> The sdap_copy_opts function copied all the arguments except for the
> sentinel.
>
> [PATCH 2/3] LDAP: Make it possible to extend an attribute map
> https://fedorahosted.org/sssd/ticket/2073
>
> This commit adds a new option ldap_user_extra_attrs that is unset by
> default. When set, the option contains a list of LDAP attributes the
> LDAP provider would download and store in addition to the usual set.
>
> The attributes are fetched in sysdb with the 'extra_' prefix to not
> conflict with the sssd sdap maps.
>
> [PATCH 3/3] Make LDAP extra attributes available to IPA and AD
> https://fedorahosted.org/sssd/ticket/2073
Looks good from 10000 feet, however I saw at least one issue, inline
>
> From cc4b347c955f93d640772fad197a55a1b9034525 Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek <jhro...@redhat.com>
> Date: Mon, 21 Apr 2014 21:33:36 +0200
> Subject: [PATCH 1/3] LDAP: Fix one-by-one bug in sdap_copy_opts
[..]
>
> + /* Include the sentinel */
> + map[num_entries].def_name = NULL;
> + map[num_entries].sys_name = NULL;
> + map[num_entries].opt_name = NULL;
> + map[num_entries].name = NULL;
Here you explicitly mark all 4 components as NULL.
[..]
Then in the next patch ...
> From 40329ebff77b0d05c8051d3d4a2af2f6545c8d86 Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek <jhro...@redhat.com>
> Date: Wed, 31 Jul 2013 10:59:43 +0200
> Subject: [PATCH 2/3] LDAP: Make it possible to extend an attribute map
[..]
> + for (nextra = 0; extra_attrs[nextra]; nextra++) ;
> + DEBUG(SSSDBG_FUNC_DATA, "%zu extra attributes\n", nextra);
(aside: is this really worth here, you can print it after the next loop
as nextra = i; at less cost)
> + map = talloc_realloc(memctx, src_map, struct sdap_attr_map,
> + num_entries + nextra + 1);
> + if (map == NULL) {
> + return ENOMEM;
> + }
> +
> + for (i = 0; extra_attrs[i]; i++) {
> + map[num_entries+i].opt_name = talloc_strdup(map,
> extra_attrs[i]);
> + map[num_entries+i].sys_name = talloc_asprintf(map,
> +
> SYSDB_EXTRA_ATTR_TMPL,
> + map[num_entries
> +i].opt_name);
> + map[num_entries+i].name = talloc_strdup(map,
> + map[num_entries
> +i].opt_name);
> + map[num_entries+i].def_name = talloc_strdup(map,
> + map[num_entries
> +i].opt_name);
> + if (map[num_entries+i].opt_name == NULL ||
> + map[num_entries+i].sys_name == NULL ||
> + map[num_entries+i].name == NULL ||
> + map[num_entries+i].def_name == NULL) {
> + return ENOMEM;
> + }
> + DEBUG(SSSDBG_TRACE_FUNC, "Extending map with %s\n",
> extra_attrs[i]);
> + }
> +
> + /* Sentinel */
> + map[num_entries+nextra].opt_name = NULL;
> + map[num_entries+nextra].sys_name = NULL;
> + map[num_entries+nextra].name = NULL;
HERE ---^^^^ only 3 elements are NULLed, def_name is missing.
I see 2 solutions:
1) only null .name
2) memset(&map[num_entries+nextra], 0, sizeof(struct sdap_attr_map));
They are both fail_safe in case struct sdap_attr_map is changed.
Simo.
>
>
--
Simo Sorce * Red Hat, Inc * New York
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
