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.