Hello,
On 2024/02/06 13:43:09 +0100, Philipp <phil...@bureaucracy.de> wrote:
> Hi
>
> As discussed on irc, an updated version with reallocarray is attached.
>
> Philipp
A few comments on this now that I had some time to properly read it.
The patch failed to apply cleanly on top of the latest master, so I had
to manually apply some parts.
> From 005b65c90f6017225c96b2769a6531057a337e70 Mon Sep 17 00:00:00 2001
> From: Philipp Takacs <phil...@bureaucracy.de>
> Date: Wed, 31 Jan 2024 17:50:52 +0100
> Subject: [PATCH 2/5] table-ldap handle more then one result
>
> ---
> configure.ac | 2 +-
> extras/tables/table-ldap/table_ldap.c | 105 ++++++++++++++++++--------
> 2 files changed, 73 insertions(+), 34 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 14608b1..181a974 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -297,7 +297,7 @@ main() { if (NSVersionOfRunTimeLibrary("System") >= (60
> << 16))
> ;;
> *-*-linux* | *-gnu* | *-k*bsd*-gnu* )
> use_pie=auto
> - CFLAGS="$CFLAGS -D_BSD_SOURCE"
> + CFLAGS="$CFLAGS -D_DEFAULT_SOURCE"
as discussed on irc this can be dropped.
I've added AC_USE_SYSTEM_EXTENSIONS in configure.ac which is a tad
better. We'll still need to add a compat for re{c,}allocarray, but i
can handle it later.
> ;;
> *-*-netbsd*)
> if test "x$withval" != "xno" ; then
> diff --git a/extras/tables/table-ldap/table_ldap.c
> b/extras/tables/table-ldap/table_ldap.c
> index 3107d44..921935e 100644
> --- a/extras/tables/table-ldap/table_ldap.c
> +++ b/extras/tables/table-ldap/table_ldap.c
> @@ -63,6 +63,10 @@ struct query {
> int attrn;
Your diff has some int-vs-size_t comparisons, assignment etc. One of
such is caused by the `attrn' field of the struct query bing int.
This seems to only be incremented, so converting it to a size_t should
be fine and gets rids of a few headaches below, but see below.
> };
>
> +struct query_result {
> + char **v[MAX_ATTRS];
> +};
> +
> static int ldap_run_query(int type, const char *, char *, size_t);
>
> static char *config, *url, *username, *password, *basedn, *ca_file;
> @@ -397,12 +401,25 @@ table_ldap_lookup(int service, struct dict *params,
> const char *key, char *dst,
> }
>
> static int
> -ldap_query(const char *filter, const char *key, char **attributes, char
> ***outp, size_t n)
> +realloc_results(struct query_result **r, size_t *num)
> +{
> + struct query_result *new;
> + size_t newsize = MAX(1, (*num)*2);
> + if ((new = reallocarray(*r, newsize, sizeof(**r))) == NULL)
> + return 0;
> + *num = newsize;
> + *r = new;
> + return 1;
> +}
> +
> +static int
> +ldap_query(const char *filter, const char *key, char **attributes, struct
> query_result **results, size_t n)
style nit: i'd prefer if the n argment was closer to attributes since
it's the attribute length.
> {
> struct aldap_message *m = NULL;
> struct aldap_page_control *pg = NULL;
> - int ret, found;
> - size_t i;
> + struct query_result *res = NULL;
> + int ret;
> + size_t i, j, found = 0, numresults = 0;
> char basedn__[MAX_LDAP_BASELEN];
> char filter__[MAX_LDAP_FILTERLEN];
> char key__[MAX_LDAP_IDENTIFIER];
> @@ -413,12 +430,11 @@ ldap_query(const char *filter, const char *key, char
> **attributes, char ***outp,
> return -1;
> if (strlcpy(key__, key, sizeof key__) >= sizeof key__)
> return -1;
> - found = -1;
> do {
> - if ((ret = aldap_search(aldap, basedn__, LDAP_SCOPE_SUBTREE,
> - filter__, key__, attributes, 0, 0, 0, pg)) == -1) {
> - log_debug("ret=%d", ret);
> - return -1;
> + ret = -1;
> + if (aldap_search(aldap, basedn__, LDAP_SCOPE_SUBTREE,
> + filter__, key__, attributes, 0, 0, 0, pg) == -1) {
> + goto error;
> }
> if (pg != NULL) {
> aldap_freepage(pg);
> @@ -433,26 +449,40 @@ ldap_query(const char *filter, const char *key, char
> **attributes, char ***outp,
> pg = m->page;
> aldap_freemsg(m);
> m = NULL;
> - if (found == -1)
> - found = 0;
> + ret = 0;
> break;
> }
> if (m->message_type != LDAP_RES_SEARCH_ENTRY)
> goto error;
>
> - found = 1;
> + if (found >= numresults) {
> + if (!realloc_results(&res, &numresults)) {
> + goto error;
> + }
> + }
> + memset(&res[found], 0, sizeof(res[found]));
> for (i = 0; i < n; ++i)
> - if (aldap_match_attr(m, attributes[i],
> &outp[i]) != 1)
> + if (aldap_match_attr(m, attributes[i],
> &res[found].v[i]) != 1)
> goto error;
> aldap_freemsg(m);
> m = NULL;
> + found++;
> }
> } while (pg != NULL);
>
> + if (ret == -1)
> + goto error;
> ret = found;
here we're setting an int to a size_t. I doubt that in practice we can
overflow, but I'd prefer this to be cleaner.
one way could be converting everything to size_t and use an extra
parameter to return the length. This may help later in
ldap_run_query(), see below.
On the other hand, we could instead keep found, numresult etc as integer
so we don't have the issue here. (and in that case, discard my previous
suggestion of turning attrn to size_t)
> + *results = res;
> goto end;
>
> error:
> + for (i = 0; i < found; i++) {
> + for (j = 0; j < n; j++) {
> + aldap_free_attr(res[i].v[j]);
> + }
> + }
> + free(res);
> ret = -1;
>
> end:
> @@ -465,9 +495,10 @@ end:
> static int
> ldap_run_query(int type, const char *key, char *dst, size_t sz)
> {
> - struct query *q;
> - char **res[4], filter[MAX_LDAP_FILTERLEN];
> - int ret, i;
> + struct query *q;
> + struct query_result *res = NULL;
> + char filter[MAX_LDAP_FILTERLEN];
> + int ret, i, j;
>
> switch (type) {
> case K_ALIAS: q = &queries[LDAP_ALIAS]; break;
> @@ -495,39 +526,40 @@ ldap_run_query(int type, const char *key, char *dst,
> size_t sz)
> return -1;
> }
>
> - memset(res, 0, sizeof(res));
> - ret = ldap_query(filter, key, q->attrs, res, q->attrn);
> + ret = ldap_query(filter, key, q->attrs, &res, q->attrn);
> if (ret <= 0 || dst == NULL)
> goto end;
>
> switch (type) {
>
> case K_ALIAS:
> + case K_MAILADDRMAP:
> memset(dst, 0, sz);
> - for (i = 0; res[0][i]; i++) {
> - if (i && strlcat(dst, ", ", sz) >= sz) {
> - ret = -1;
> - break;
> - }
> - if (strlcat(dst, res[0][i], sz) >= sz) {
> - ret = -1;
> - break;
> + for (j = 0; j < ret; j++) {
> + for (i = 0; res[j].v[0][i]; i++) {
> + if ((i || j) && strlcat(dst, ", ", sz) >= sz) {
> + ret = -1;
You're setting ret to -1 and exiting the inner loop, but the outer loop
is still using ret...
separating the return value of ldap_query from the number of results
would be useful here too.
> + break;
> + }
> + if (strlcat(dst, res[j].v[0][i], sz) >= sz) {
> + ret = -1;
ditto
> + break;
> + }
> }
> }
> break;
> case K_DOMAIN:
> case K_MAILADDR:
> - case K_MAILADDRMAP:
> - if (strlcpy(dst, res[0][0], sz) >= sz)
> + if (strlcpy(dst, res[0].v[0][0], sz) >= sz)
> ret = -1;
> break;
> case K_CREDENTIALS:
> - if (snprintf(dst, sz, "%s:%s", res[0][0], res[1][0]) >= (int)sz)
> + if (snprintf(dst, sz, "%s:%s", res[0].v[0][0], res[0].v[1][0])
> >= (int)sz)
> ret = -1;
> break;
> case K_USERINFO:
> - if (snprintf(dst, sz, "%s:%s:%s", res[0][0], res[1][0],
> - res[2][0]) >= (int)sz)
> + if (snprintf(dst, sz, "%s:%s:%s", res[0].v[0][0],
> res[0].v[1][0],
> + res[0].v[2][0]) >= (int)sz)
> ret = -1;
> break;
> default:
> @@ -539,10 +571,17 @@ ldap_run_query(int type, const char *key, char *dst,
> size_t sz)
> log_warnx("warn: could not format result");
>
> end:
> - for (i = 0; i < q->attrn; ++i)
> - if (res[i])
> - aldap_free_attr(res[i]);
> + for (j = 0; j < ret; ++j) {
> + for (i = 0; i < q->attrn; ++i) {
nit: it's a bit confusing that here i and j are used in the inverse
order of the cleanup in ldap_query.
> + if (res[j].v[i]) {
> + aldap_free_attr(res[j].v[i]);
> + }
> + }
> + }
> [...]
Thanks,
Omar Polo
