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

Jan Engelhardt jengelh at inai.de
Fri Feb 22 17:33:55 UTC 2013


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

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

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

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



More information about the sssd-devel mailing list