[SSSD] [PATCH] sysdb: try dealing with binary-content attributes

Jakub Hrozek jhrozek at redhat.com
Fri Feb 22 16:31:55 UTC 2013


On Thu, Feb 21, 2013 at 01:20:47PM +0100, Jan Engelhardt wrote:
> I have here a LDAP user entry which has this attribute
> 
> 	loginAllowedTimeMap::
> 	 AAAAAAAAAP///38AAP///38AAP///38AAP///38AAP///38AAAAAAAAA
> 
> In the function sysdb_attrs_add_string(), called from
> sdap_attrs_add_ldap_attr(), strlen() is used to populate the .v_length
> member of a struct ldb_val - and this will set it to zero.
> 

Hi,

in general I think the patch is correct. We shouldn't be relying on
having a null-terminated string incoming from LDAP when saving a generic
attribute (unlike the ldb attribute then. IIRC the ldb attributes are
guaranteed to be NULL-terminated). I only have a couple of nitpicks, see
inline.

Thank you very much for tracking this problem down and contributing the
patch.

> Subsequently, this makes ldb_modify(), called from
> sysdb_set_entry_attr(), return LDB_ERR_INVALID_ATTRIBUTE_SYNTAX.
> ---
>  src/db/sysdb.c                  |   10 ++++++++++
>  src/db/sysdb.h                  |    2 ++
>  src/providers/ldap/sdap_async.c |    4 ++--
>  3 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/src/db/sysdb.c b/src/db/sysdb.c
> index e7524f4..7c34791 100644
> --- a/src/db/sysdb.c
> +++ b/src/db/sysdb.c
> @@ -512,6 +512,16 @@ int sysdb_attrs_add_string(struct sysdb_attrs *attrs,
>      return sysdb_attrs_add_val(attrs, name, &v);
>  }
>  
> +int sysdb_attrs_add_mem(struct sysdb_attrs *attrs, const char *name,

Since we use talloc all around the place in the sssd, the _mem suffix
evocates that this function would add contents of a memory context, but
it's more of a "generic value coming from LDAP", so I was wondering if
_buf of _blob might have been better suited. But this is not a blocker,
more like thinking out loud.

> +                        const void *mem, size_t size)

For a similar reason, I would have personally used uint8_t * here, which
would also match the data type commonly used as input to this function.

> +{
> +	struct ldb_val v;
> +
> +	v.data   = discard_const(mem);
> +	v.length = size;
> +	return sysdb_attrs_add_val(attrs, name, &v);
> +}
> +
>  int sysdb_attrs_add_bool(struct sysdb_attrs *attrs,
>                           const char *name, bool value)
>  {
> diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> index fff97a8..23cbbb0 100644
> --- a/src/db/sysdb.h
> +++ b/src/db/sysdb.h
> @@ -250,6 +250,8 @@ int sysdb_attrs_add_val(struct sysdb_attrs *attrs,
>                          const char *name, const struct ldb_val *val);
>  int sysdb_attrs_add_string(struct sysdb_attrs *attrs,
>                             const char *name, const char *str);
> +int sysdb_attrs_add_mem(struct sysdb_attrs *, const char *,
> +                        const void *, size_t);

Can you use the parameter names, too in the declaration? That way it
would be consistent with the rest of the header file.

>  int sysdb_attrs_add_bool(struct sysdb_attrs *attrs,
>                           const char *name, bool value);
>  int sysdb_attrs_add_long(struct sysdb_attrs *attrs,
> diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
> index 84497b7..b7d9839 100644
> --- a/src/providers/ldap/sdap_async.c
> +++ b/src/providers/ldap/sdap_async.c
> @@ -2226,8 +2226,8 @@ sdap_attrs_add_ldap_attr(struct sysdb_attrs *ldap_attrs,
>              DEBUG(SSSDBG_TRACE_INTERNAL, ("Adding %s [%s] to attributes "
>                    "of [%s].\n", desc, el->values[i].data, objname));
>  
> -            ret = sysdb_attrs_add_string(attrs, attr_name,
> -                                         (const char *) el->values[i].data);
> +            ret = sysdb_attrs_add_mem(attrs, attr_name, el->values[i].data,
> +                                      el->values[i].length);

This usage is correct, but I checked out the codebase and I think the
function could also be used in sdap_parse_deref() around line 480 in the
current master.

>              if (ret) {
>                  return ret;
>              }
> -- 
> 1.7.10.4



More information about the sssd-devel mailing list