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

Jakub Hrozek jhrozek at redhat.com
Mon Feb 25 10:29:26 UTC 2013


On Fri, Feb 22, 2013 at 06:33:55PM +0100, Jan Engelhardt wrote:
> 
> On Friday 2013-02-22 17:31, Jakub Hrozek 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.
> >
> >in general I think the patch is correct. We shouldn't be relying on
> >having a null-terminated string incoming from LDAP [...]
> >IIRC the ldb attributes are
> >guaranteed to be NULL-terminated
> 
> My problem was not the lack of termination, but that something that
> is not a logical string was fed to a string function. :)
> 

Ah, sorry, then I misunderstood you on the IRC. I was under the
impression that strlen() returned bogus data b/c the attribute was not
null-terminated.

> (That is to say, something that is NUL(L)-terminated need not
> necessarily be a string. Could as well be an array of integers
> with a sentinel value of 0.)
> 

Indeed.

> >> +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
> 
> Well it does add contents of a memory region, does it not? LDB does
> not play a role here. I simply reused the terminology from the C
> runtime which has, for example, strcpy and memcpy, and that also is
> the justification for using void *. The function really is dealing
> with arbitrary data.

Well, "arbitrary data" implies even C structures to me and I doubt
they'd be stored in LDAP :-)

As I said, this is mostly a matter of personal preference, though. I'll
accept the patch even with _mem and void*.

> On the other hand, sysdb_attrs_add_string should
> IMO be using a const char *, not uint8_t *, because on a logical
> level, it operates on strings, not an array of integers.
> 
> (I notice that struct ldb_val also uses uint8_t, so it is probably
> them who need to be blamed.)
> 

Fair enough. 

> >>              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.
> 
> That seems indeed so.

Are you going to send another iteration that uses the new call in
sdap_parse_deref() or shall I do it on your behalf?



More information about the sssd-devel mailing list