[2024-01-24 00:09] Omar Polo <o...@omarpolo.com>
> On 2024/01/23 19:49:34 +0100, Philipp <phil...@bureaucracy.de> wrote:
> > [2024-01-23 11:39] Omar Polo <o...@omarpolo.com>
> > > spotted while reading Philipp' ldaps diff. it's really ugly to reach
> > > into the struct sockaddrs when using getaddrinfo()...
> >
> > Nice this makes the libtls integration simpler. Also some comments inline.
> >
> > > however, I don't use ldap so this could use at least some testing :)
> >
> > No problem I can test this.
>
> thanks :)
No problem. I'm currently working on a new mailserver, I need these changes
for. My current plan is to test all this tomorrow.
> even if we'll change the transport away from imsg, it could still be
> useful to improve these extras I think. (again, don't know much of ldap,
> nor can judge the state of tha table-ldap; just scratching an itch after
> taking a look at your diff.)
>
> > > @@ -85,8 +85,8 @@ ldap_connect(const char *addr)
> > > {
> > > struct aldap_url lu;
> > > struct addrinfo hints, *res0, *res;
> > > - char *buf;
> > > - int error, fd = -1;
> > > + char *buf, port[32];
> >
> > nitpick: the port is max 65535, so 8 byte would be enough.
>
> right, i picked the first power of two that came to mind. it could be
> interesting however to change the parsing function to keep the port as
> string, to avoid this conversion here.
Nice Idea, patch is attached.
> > [...]
> > > + if (connect(fd, res->ai_addr, res->ai_addrlen) == 0)
> > > + return aldap_init(fd);
> >
> > Here a aldap_free_url() is missing, therefor lu.buffer is leaking.
> > But currently aldap_free_url() is buggy, it frees lu->buffer and
> > lu->filter but this is the same object.
>
> Yes, but it's the same behaviour as before. I didn't want to change
> many things at a time.
>
> if you're interested in this however, we can also avoid the strdup()
> here since aldap_parse_url() already strdup()s the string for parsing
> (but still frees the passed argument...)
I have written two patches for this, one adding the free and one to
avoid the unnecessary strdup.
Ass you might guess from the filenames, there are a few more patches. I'll
send the rest after I have tested all my patches.
From fa4cdb0a74c3b5d17cdc93b6285d765fda084740 Mon Sep 17 00:00:00 2001
From: Philipp Takacs <phil...@bureaucracy.de>
Date: Wed, 24 Jan 2024 01:16:56 +0100
Subject: [PATCH 11/11] table-ldap aldap_parse_url now saves the port as string
---
extras/tables/table-ldap/aldap.c | 3 ++-
extras/tables/table-ldap/aldap.h | 6 +++---
extras/tables/table-ldap/table_ldap.c | 11 ++---------
3 files changed, 7 insertions(+), 13 deletions(-)
diff --git a/extras/tables/table-ldap/aldap.c b/extras/tables/table-ldap/aldap.c
index 5907c6e..7058d81 100644
--- a/extras/tables/table-ldap/aldap.c
+++ b/extras/tables/table-ldap/aldap.c
@@ -601,9 +601,10 @@ aldap_parse_url(const char *url, struct aldap_url *lu)
/* if a port is given */
if (*(forward2+1) != '\0') {
#define PORT_MAX UINT16_MAX
- lu->port = strtonum(++forward2, 0, PORT_MAX, &errstr);
+ strtonum(++forward2, 0, PORT_MAX, &errstr);
if (errstr)
goto fail;
+ lu->port = forward2;
}
} else {
lu->port = LDAP_PORT;
diff --git a/extras/tables/table-ldap/aldap.h b/extras/tables/table-ldap/aldap.h
index 088ee60..60159b4 100644
--- a/extras/tables/table-ldap/aldap.h
+++ b/extras/tables/table-ldap/aldap.h
@@ -19,9 +19,9 @@
#include "ber.h"
#define LDAP_URL "ldap://";
-#define LDAP_PORT 389
+#define LDAP_PORT "389"
#define LDAPS_URL "ldaps://"
-#define LDAPS_PORT 636
+#define LDAPS_PORT "636"
#define LDAP_PAGED_OID "1.2.840.113556.1.4.319"
struct aldap {
@@ -73,7 +73,7 @@ enum aldap_protocol {
struct aldap_url {
enum aldap_protocol protocol;
char *host;
- in_port_t port;
+ char *port;
char *dn;
#define MAXATTR 1024
char *attributes[MAXATTR];
diff --git a/extras/tables/table-ldap/table_ldap.c b/extras/tables/table-ldap/table_ldap.c
index 0fb5fe3..3c6437a 100644
--- a/extras/tables/table-ldap/table_ldap.c
+++ b/extras/tables/table-ldap/table_ldap.c
@@ -118,7 +118,6 @@ ldap_connect(const char *addr)
{
struct aldap_url lu;
struct addrinfo hints, *res0, *res;
- char port[32];
int error, r, fd = -1;
if (aldap_parse_url(addr, &lu) != 1) {
@@ -126,22 +125,16 @@ ldap_connect(const char *addr)
return NULL;
}
- r = snprintf(port, sizeof(port), "%d", lu.port);
- if (r < 0 || (size_t)r >= sizeof(port)) {
- log_warnx("snprintf");
- return NULL;
- }
-
memset(&hints, 0, sizeof(hints));
hints.ai_family = PF_UNSPEC;
hints.ai_socktype = SOCK_STREAM;
hints.ai_flags = AI_NUMERICSERV;
- error = getaddrinfo(lu.host, port, &hints, &res0);
+ error = getaddrinfo(lu.host, lu.port, &hints, &res0);
if (error == EAI_AGAIN || error == EAI_NODATA || error == EAI_NONAME)
return NULL;
if (error) {
log_warnx("warn: could not parse \"%s:%s\": %s", lu.host,
- port, gai_strerror(error));
+ lu.port, gai_strerror(error));
return NULL;
}
--
2.39.2
From 295672ebf212cfa781d2a7c410efaf54fe482a2b Mon Sep 17 00:00:00 2001
From: Philipp Takacs <phil...@bureaucracy.de>
Date: Tue, 23 Jan 2024 13:58:47 +0100
Subject: [PATCH 02/11] table-ldap free aldap_url after usage
---
extras/tables/table-ldap/aldap.c | 1 -
extras/tables/table-ldap/table_ldap.c | 5 ++++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/extras/tables/table-ldap/aldap.c b/extras/tables/table-ldap/aldap.c
index d54a90c..552ea52 100644
--- a/extras/tables/table-ldap/aldap.c
+++ b/extras/tables/table-ldap/aldap.c
@@ -560,7 +560,6 @@ void
aldap_free_url(struct aldap_url *lu)
{
free(lu->buffer);
- free(lu->filter);
}
int
diff --git a/extras/tables/table-ldap/table_ldap.c b/extras/tables/table-ldap/table_ldap.c
index 6bdce67..79a26a8 100644
--- a/extras/tables/table-ldap/table_ldap.c
+++ b/extras/tables/table-ldap/table_ldap.c
@@ -122,13 +122,16 @@ ldap_connect(const char *addr)
if (fd == -1)
continue;
- if (connect(fd, res->ai_addr, res->ai_addrlen) == 0)
+ if (connect(fd, res->ai_addr, res->ai_addrlen) == 0) {
+ aldap_free_url(&lu);
return aldap_init(fd);
+ }
close(fd);
fd = -1;
}
+ aldap_free_url(&lu);
return NULL;
}
--
2.39.2
From 351ef9581e10223617a18f1da3a5078b587aae23 Mon Sep 17 00:00:00 2001
From: Philipp Takacs <phil...@bureaucracy.de>
Date: Tue, 23 Jan 2024 22:04:14 +0100
Subject: [PATCH 09/11] table-ldap don't free addr in aldap_parse_url
aldap_parse_url() already copy the url to work on.
---
extras/tables/table-ldap/aldap.c | 3 +--
extras/tables/table-ldap/aldap.h | 2 +-
extras/tables/table-ldap/table_ldap.c | 9 ++-------
3 files changed, 4 insertions(+), 10 deletions(-)
diff --git a/extras/tables/table-ldap/aldap.c b/extras/tables/table-ldap/aldap.c
index edd3cc3..5aa288a 100644
--- a/extras/tables/table-ldap/aldap.c
+++ b/extras/tables/table-ldap/aldap.c
@@ -571,7 +571,7 @@ aldap_free_url(struct aldap_url *lu)
}
int
-aldap_parse_url(char *url, struct aldap_url *lu)
+aldap_parse_url(const char *url, struct aldap_url *lu)
{
char *p, *forward, *forward2;
const char *errstr = NULL;
@@ -675,7 +675,6 @@ aldap_parse_url(char *url, struct aldap_url *lu)
if (p)
lu->filter = p;
done:
- free(url);
return (1);
fail:
free(lu->buffer);
diff --git a/extras/tables/table-ldap/aldap.h b/extras/tables/table-ldap/aldap.h
index fec106b..2508c07 100644
--- a/extras/tables/table-ldap/aldap.h
+++ b/extras/tables/table-ldap/aldap.h
@@ -203,7 +203,7 @@ char *aldap_get_dn(struct aldap_message *);
char *aldap_get_diagmsg(struct aldap_message *);
char **aldap_get_references(struct aldap_message *);
void aldap_free_references(char **values);
-int aldap_parse_url(char *, struct aldap_url *);
+int aldap_parse_url(const char *, struct aldap_url *);
void aldap_free_url(struct aldap_url *);
#if 0
int aldap_search_url(struct aldap *, char *, int, int, int);
diff --git a/extras/tables/table-ldap/table_ldap.c b/extras/tables/table-ldap/table_ldap.c
index ea37bac..fbd9f55 100644
--- a/extras/tables/table-ldap/table_ldap.c
+++ b/extras/tables/table-ldap/table_ldap.c
@@ -118,16 +118,11 @@ ldap_connect(const char *addr)
{
struct aldap_url lu;
struct addrinfo hints, *res0, *res;
- char *buf, port[32];
+ char port[32];
int error, r, fd = -1;
- if ((buf = strdup(addr)) == NULL)
- return NULL;
-
- /* aldap_parse_url frees buf on success */
- if (aldap_parse_url(buf, &lu) != 1) {
+ if (aldap_parse_url(addr, &lu) != 1) {
log_warnx("warn: ldap_parse_url fail");
- free(buf);
return NULL;
}
--
2.39.2
