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.
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
