Hi
Thanks for your feedback. Updated patches are attached.
[2024-02-10 13:37] Omar Polo <o...@omarpolo.com>
> 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.
Not sure why, either because I have them on top of my ldaps patches or
because you forgott the "request only required attributes" patch. Because
I haven't change this one I haven't add it again. The atteched version is
direct on master and all patches are attached.
> > 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
> >
> > ---
> [...]
> > @@ -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.
I have changed this to size_t.
> > @@ -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.
I have reorderd the arguments. Also changed the naming from 'n' to
'nattr'. This makes the code more readable.
> > {
> > 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)
I more like size_t way, so I have added a *numresults argument.
> > @@ -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...
And later the free also using it, fixed this.
> separating the return value of ldap_query from the number of results
> would be useful here too.
See above.
> [...]
> > @@ -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.
Yes, I was a bit lacy. Changed it consequently to i before j.
> [...]
I haven't tested this yet, I plan to test it on the weekend.
Philipp
From 4ca89a68f09324bbc736173a3846c43c03772c8a Mon Sep 17 00:00:00 2001
From: Philipp Takacs <phil...@bureaucracy.de>
Date: Tue, 30 Jan 2024 17:07:07 +0100
Subject: [PATCH 1/2] table-ldap request only required attributes
---
extras/tables/table-ldap/table_ldap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/extras/tables/table-ldap/table_ldap.c b/extras/tables/table-ldap/table_ldap.c
index 64f99a8..bdd5d9c 100644
--- a/extras/tables/table-ldap/table_ldap.c
+++ b/extras/tables/table-ldap/table_ldap.c
@@ -366,7 +366,7 @@ ldap_query(const char *filter, const char *key, char **attributes, char ***outp,
found = -1;
do {
if ((ret = aldap_search(aldap, basedn__, LDAP_SCOPE_SUBTREE,
- filter__, key__, NULL, 0, 0, 0, pg)) == -1) {
+ filter__, key__, attributes, 0, 0, 0, pg)) == -1) {
log_debug("ret=%d", ret);
return -1;
}
--
2.39.2
From 4bfe8ee070de3eec79c06e8c94b0d15aa90278b5 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/2] table-ldap handle more then one result
---
configure.ac | 2 +-
extras/tables/table-ldap/table_ldap.c | 124 +++++++++++++++++---------
2 files changed, 81 insertions(+), 45 deletions(-)
diff --git a/configure.ac b/configure.ac
index 7ecc50d..1af7742 100644
--- a/configure.ac
+++ b/configure.ac
@@ -299,7 +299,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"
;;
*-*-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 bdd5d9c..bf84dc6 100644
--- a/extras/tables/table-ldap/table_ldap.c
+++ b/extras/tables/table-ldap/table_ldap.c
@@ -59,7 +59,11 @@ enum {
struct query {
char *filter;
char *attrs[MAX_ATTRS];
- int attrn;
+ size_t attrn;
+};
+
+struct query_result {
+ char **v[MAX_ATTRS];
};
static int ldap_run_query(int type, const char *, char *, size_t);
@@ -347,12 +351,26 @@ 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, size_t nattr,
+ struct query_result **results, size_t *nresults)
{
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, nres = 0;
char basedn__[MAX_LDAP_BASELEN];
char filter__[MAX_LDAP_FILTERLEN];
char key__[MAX_LDAP_IDENTIFIER];
@@ -363,12 +381,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 end;
}
if (pg != NULL) {
aldap_freepage(pg);
@@ -377,35 +394,46 @@ ldap_query(const char *filter, const char *key, char **attributes, char ***outp,
while ((m = aldap_parse(aldap)) != NULL) {
if (aldap->msgid != m->msgid)
- goto error;
+ goto end;
if (m->message_type == LDAP_RES_SEARCH_RESULT) {
if (m->page != NULL && m->page->cookie_len)
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;
+ goto end;
- found = 1;
- for (i = 0; i < n; ++i)
- if (aldap_match_attr(m, attributes[i], &outp[i]) != 1)
- goto error;
+ if (found >= nres) {
+ if (!realloc_results(&res, &nres)) {
+ goto end;
+ }
+ }
+ memset(&res[found], 0, sizeof(res[found]));
+ for (i = 0; i < nattr; ++i)
+ if (aldap_match_attr(m, attributes[i], &res[found].v[i]) != 1)
+ goto end;
aldap_freemsg(m);
m = NULL;
+ found++;
}
} while (pg != NULL);
- ret = found;
- goto end;
-
-error:
- ret = -1;
-
end:
+ if (ret == -1) {
+ for (i = 0; i < found; i++) {
+ for (j = 0; j < nattr; j++) {
+ aldap_free_attr(res[i].v[j]);
+ }
+ }
+ free(res);
+ } else {
+ ret = found ? 1 : 0;
+ *nresults = found;
+ *results = res;
+ }
if (m)
aldap_freemsg(m);
log_debug("debug: table_ldap: ldap_query: filter=%s, ret=%d", filter, ret);
@@ -415,9 +443,11 @@ 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;
+ size_t i, j, nres = 0;
+ char filter[MAX_LDAP_FILTERLEN];
+ int ret;
switch (type) {
case K_ALIAS: q = &queries[LDAP_ALIAS]; break;
@@ -445,39 +475,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, q->attrn, &res, &nres);
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 (i = 0; ret == 1 && i < nres; i++) {
+ for (j = 0; ret == 1 && res[i].v[0][j]; j++) {
+ if ((i || j) && strlcat(dst, ", ", sz) >= sz) {
+ ret = -1;
+ break;
+ }
+ if (strlcat(dst, res[i].v[0][j], sz) >= sz) {
+ ret = -1;
+ 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:
@@ -489,9 +520,14 @@ 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 (i = 0; i < nres; ++j) {
+ for (j = 0; j < q->attrn; ++i) {
+ if (res[i].v[j]) {
+ aldap_free_attr(res[i].v[j]);
+ }
+ }
+ }
+ free(res);
return ret;
}
--
2.39.2
