Re: opensmtpd-extra: simplify getaddrinfo() usage
Hi
sorry I have mixed up my patch files, I have attached the correct ones.
Philipp
[2024-01-25 13:16] Philipp Takacs
> [2024-01-25 10:15] Omar Polo
> > On 2024/01/24 08:51:06 +0100, Philipp wrote:
> > > [2024-01-24 00:09] Omar Polo
> > > > [...]
> > > > 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.
> >
> > They all read fine to me. only one nitpick in the first one
> >
> > > From fa4cdb0a74c3b5d17cdc93b6285d765fda084740 Mon Sep 17 00:00:00 2001
> > > From: Philipp Takacs
> > > Date: Wed, 24 Jan 2024 01:16:56 +0100
> > > Subject: [PATCH 11/11] table-ldap aldap_parse_url now saves the port as
> > > string
> > > [...]
> > > --- 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;
> >
> > Here you could remove `r' too since it becomes unused.
>
> Thanks fixed.
>
> > If you've tested, I could commit my getaddrinfo() diff, then if you
> > rebase these one on top of it I could merge them. (but it's also fine to
> > get the ldaps one in first, depending on how you prefer.)
>
> I have tested now your getaddrinfo rewrite and my two fixups.
> I have had some changes because they had been somewere in
> my rebase branche. clean versions of them are attached.
>
> Philipp
From d4fa732b5375e0fa8eaa0727ae82362c941bf8c1 Mon Sep 17 00:00:00 2001
From: Philipp Takacs
Date: Tue, 23 Jan 2024 13:58:47 +0100
Subject: [PATCH 2/3] table-ldap free aldap_url
---
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 dafbf8547e57b0f7142c8faf6ef776933502d151 Mon Sep 17 00:00:00 2001
From: Philipp Takacs
Date: Thu, 25 Jan 2024 12:47:46 +0100
Subject: [PATCH 3/3] table-ldap aldap_parse_url now saves the port as string
---
extras/tables/table-ldap/aldap.c | 3 ++-
extras/tables/table-ldap/aldap.h | 4 ++--
extras/tables/table-ldap/table_ldap.c | 14 --
3 files changed, 8 insertions(+), 13 deletions(-)
diff --git a/extras/tables/table-ldap/aldap.c b/extras/tables/table-ldap/aldap.c
index 552ea52..011a820 100644
--- a/extras/tables/table-ldap/aldap.c
+++ b/extras/tables/table-ldap/aldap.c
@@ -588,9 +588,10 @@ aldap_parse_url(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 7cfd637..7217634 100644
--- a/extras/tables/table-ldap/aldap.h
+++ b/extras/tables/table-ldap/aldap.h
@@ -19,7 +19,7 @@
#include "ber.h"
#define LDAP_URL "ldap://";
-#define LDAP_PORT 389
+#define LDAP_PORT "389"
#define LDAP_PAGED_OID "1.2.840.113556.1.4.319"
struct aldap {
@@ -71,7 +71,7 @@ enum aldap_protocol {
struct aldap_url {
int 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 79a26a8..0f25c60 100644
--- a/extras/tables/table-ldap/table_ldap.c
+++ b/extras/tables/table-ldap/table_ldap.c
@@ -85,8 +85,8 @@ ldap_connect(const char *addr)
{
struct aldap_url lu;
struct addrinfo hints, *res0, *res;
- char *buf, port[32];
- int error, r, fd = -1;
+ char *buf;
+ int error, fd = -1;
if
Re: opensmtpd-extra: simplify getaddrinfo() usage
[2024-01-25 10:15] Omar Polo
> On 2024/01/24 08:51:06 +0100, Philipp wrote:
> > [2024-01-24 00:09] Omar Polo
> > > [...]
> > > 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.
>
> They all read fine to me. only one nitpick in the first one
>
> > From fa4cdb0a74c3b5d17cdc93b6285d765fda084740 Mon Sep 17 00:00:00 2001
> > From: Philipp Takacs
> > Date: Wed, 24 Jan 2024 01:16:56 +0100
> > Subject: [PATCH 11/11] table-ldap aldap_parse_url now saves the port as
> > string
> > [...]
> > --- 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;
>
> Here you could remove `r' too since it becomes unused.
Thanks fixed.
> If you've tested, I could commit my getaddrinfo() diff, then if you
> rebase these one on top of it I could merge them. (but it's also fine to
> get the ldaps one in first, depending on how you prefer.)
I have tested now your getaddrinfo rewrite and my two fixups.
I have had some changes because they had been somewere in
my rebase branche. clean versions of them are attached.
Philipp
From d4fa732b5375e0fa8eaa0727ae82362c941bf8c1 Mon Sep 17 00:00:00 2001
From: Philipp Takacs
Date: Tue, 23 Jan 2024 13:58:47 +0100
Subject: [PATCH 2/3] table-ldap free aldap_url
---
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 a7c50eb250bfbb79957b4edf615cdec3dd96560f Mon Sep 17 00:00:00 2001
From: Philipp Takacs
Date: Thu, 25 Jan 2024 12:47:46 +0100
Subject: [PATCH 3/3] table-ldap aldap_parse_url now saves the port as string
---
extras/tables/table-ldap/aldap.c | 3 ++-
extras/tables/table-ldap/aldap.h | 4 ++--
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/extras/tables/table-ldap/aldap.c b/extras/tables/table-ldap/aldap.c
index 552ea52..011a820 100644
--- a/extras/tables/table-ldap/aldap.c
+++ b/extras/tables/table-ldap/aldap.c
@@ -588,9 +588,10 @@ aldap_parse_url(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 7cfd637..7217634 100644
--- a/extras/tables/table-ldap/aldap.h
+++ b/extras/tables/table-ldap/aldap.h
@@ -19,7 +19,7 @@
#include "ber.h"
#define LDAP_URL "ldap://";
-#define LDAP_PORT 389
+#define LDAP_PORT "389"
#define LDAP_PAGED_OID "1.2.840.113556.1.4.319"
struct aldap {
@@ -71,7 +71,7 @@ enum aldap_protocol {
struct aldap_url {
int protocol;
char *host;
- in_port_t port;
+ char *port;
char *dn;
#define MAXATTR 1024
char *attributes[MAXATTR];
--
2.39.2
Re: opensmtpd-extra: simplify getaddrinfo() usage
On 2024/01/24 08:51:06 +0100, Philipp wrote:
> [2024-01-24 00:09] Omar Polo
> > [...]
> > 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.
They all read fine to me. only one nitpick in the first one
> From fa4cdb0a74c3b5d17cdc93b6285d765fda084740 Mon Sep 17 00:00:00 2001
> From: Philipp Takacs
> Date: Wed, 24 Jan 2024 01:16:56 +0100
> Subject: [PATCH 11/11] table-ldap aldap_parse_url now saves the port as string
> [...]
> --- 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;
Here you could remove `r' too since it becomes unused.
If you've tested, I could commit my getaddrinfo() diff, then if you
rebase these one on top of it I could merge them. (but it's also fine to
get the ldaps one in first, depending on how you prefer.)
Thanks,
Omar Polo
Re: opensmtpd-extra: simplify getaddrinfo() usage
[2024-01-24 00:09] Omar Polo
> On 2024/01/23 19:49:34 +0100, Philipp wrote:
> > [2024-01-23 11:39] Omar Polo
> > > 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
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.
Re: opensmtpd-extra: simplify getaddrinfo() usage
On 2024/01/23 19:49:34 +0100, Philipp wrote:
> [2024-01-23 11:39] Omar Polo
> > 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 :)
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.
> [...]
> > + 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...)
Re: opensmtpd-extra: simplify getaddrinfo() usage
[2024-01-23 11:39] Omar Polo
> 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.
> (would also be interesting to provide some more logging if socket/connect
> fails somehow, and also aldap_parse_url could use some simplifying.
> don't want to fall too much into this rabbit hole though)
>
> diff /home/op/w/opensmtpd-extras
> commit - 5715b1ff87eafd465592df5c2cf4b2f171e60bbc
> path + /home/op/w/opensmtpd-extras
> blob - 090cfb467a79c71c8d28ad9f75e6a0faf859cdd8
> file + extras/tables/table-ldap/table_ldap.c
> --- extras/tables/table-ldap/table_ldap.c
> +++ extras/tables/table-ldap/table_ldap.c
> @@ -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.
> + int error, r, fd = -1;
>
> if ((buf = strdup(addr)) == NULL)
> return NULL;
> @@ -98,37 +98,32 @@ 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; /* DUMMY */
> - error = getaddrinfo(lu.host, NULL, &hints, &res0);
> + hints.ai_socktype = SOCK_STREAM;
> + hints.ai_flags = AI_NUMERICSERV;
> + error = getaddrinfo(lu.host, 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", lu.host,
> - gai_strerror(error));
> + log_warnx("warn: could not parse \"%s:%s\": %s", lu.host,
> + port, gai_strerror(error));
> return NULL;
> }
>
> for (res = res0; res; res = res->ai_next) {
> - if (res->ai_family != AF_INET && res->ai_family != AF_INET6)
> - continue;
> -
> fd = socket(res->ai_family, res->ai_socktype, res->ai_protocol);
> if (fd == -1)
> continue;
>
> - if (res->ai_family == AF_INET) {
> - struct sockaddr_in sin4 = *(struct sockaddr_in
> *)res->ai_addr;
> - sin4.sin_port = htons(lu.port);
> - if (connect(fd, (struct sockaddr *)&sin4,
> res->ai_addrlen) == 0)
> - return aldap_init(fd);
> - } else if (res->ai_family == AF_INET6) {
> - struct sockaddr_in6 sin6 = *(struct sockaddr_in6
> *)res->ai_addr;
> - sin6.sin6_port = htons(lu.port);
> - if (connect(fd, (struct sockaddr *)&sin6,
> res->ai_addrlen) == 0)
> - return aldap_init(fd);
> - }
> + 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.
>
> close(fd);
> fd = -1;
>
