Re: [patch] Re: Possible sasyncd memory leak ?
On Tue, Mar 26, 2019 at 03:52:47PM +0100, Michał Koc wrote:
> W dniu 25.03.2019 o 15:08, Otto Moerbeek pisze:
> > On Sat, Mar 23, 2019 at 06:07:02PM +0100, Michał Koc wrote:
> >
> > > ... [snip]
> > This is almost good. You might fold host_ip() into net_set_sa(). the
> > double malloc and copy isn't really needed.
> >
> > -Otto
> >
> Done, patch follows
>
> Best regards
> M.K
>
>
Looks good to me,
Klemens, can you take a look?
-Otto
> Index: conf.y
> ===
> RCS file: /cvs/src/usr.sbin/sasyncd/conf.y,v
> retrieving revision 1.19
> diff -u -p -r1.19 conf.y
> --- conf.y9 Apr 2017 02:40:24 - 1.19
> +++ conf.y26 Mar 2019 14:51:52 -
> @@ -32,6 +32,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -48,6 +49,7 @@ struct cfgstate cfgstate;
> int conflen = 0;
> char *confbuf, *confptr;
>
> +int check_peer_addr(const char *);
> int yyparse(void);
> int yylex(void);
> void yyerror(const char *);
> @@ -172,29 +174,21 @@ setting : INTERFACE STRING
> | PEER STRING
> {
> struct syncpeer *peer;
> - int duplicate = 0;
>
> - for (peer = LIST_FIRST(&cfgstate.peerlist); peer;
> - peer = LIST_NEXT(peer, link))
> - if (strcmp($2, peer->name) == 0) {
> - duplicate++;
> - break;
> - }
> - if (duplicate)
> - free($2);
> - else {
> + if (check_peer_addr($2)) {
> peer = calloc(1, sizeof *peer);
> - if (!peer) {
> + if (peer == NULL) {
> log_err("config: calloc(1, %lu) "
> "failed", sizeof *peer);
> free($2);
> YYERROR;
> }
> peer->name = $2;
> - }
> - LIST_INSERT_HEAD(&cfgstate.peerlist, peer, link);
> - cfgstate.peercnt++;
> - log_msg(2, "config: add peer %s", peer->name);
> + LIST_INSERT_HEAD(&cfgstate.peerlist, peer,
> link);
> + cfgstate.peercnt++;
> + log_msg(2, "config: add peer %s", peer->name);
> + } else
> + free($2);
> }
> | LISTEN ON STRING af port
> {
> @@ -281,6 +275,46 @@ match(char *token)
> sizeof keywords[0], match_cmp);
>
> return k ? k->value : STRING;
> +}
> +
> +int
> +check_peer_addr(const char *peer_addr)
> +{
> + struct ifaddrs *ifap = 0, *ifa;
> + struct syncpeer *peer;
> + struct sockaddr_storage ss, peer_ss;
> +
> + if(net_set_sa((struct sockaddr *)&ss, peer_addr, 0) == -1) {
> + log_msg(2, "config: skip unparseable peer %s", peer_addr);
> + return 0;
> + }
> +
> + if (getifaddrs(&ifap) == 0) {
> + for (ifa = ifap; ifa != NULL; ifa = ifa->ifa_next) {
> + if (ifa->ifa_addr == NULL || ifa->ifa_addr->sa_family
> != ss.ss_family)
> + continue;
> +
> + if (ss.ss_len == ifa->ifa_addr->sa_len && memcmp(&ss,
> ifa->ifa_addr, ss.ss_len) == 0) {
> + log_msg(2, "config: skip local peer %s",
> peer_addr);
> + freeifaddrs(ifap);
> + return 0;
> + }
> + }
> + freeifaddrs(ifap);
> + }
> +
> + for (peer = LIST_FIRST(&cfgstate.peerlist); peer != NULL; peer =
> LIST_NEXT(peer, link)) {
> + if(net_set_sa((struct sockaddr *)&peer_ss, peer->name, 0) == -1) {
> + log_msg(2, "config: net_set_sa(%s) failed", peer->name);
> + continue;
> + }
> + if (ss.ss_len == peer_ss.ss_len && memcmp(&ss, &peer_ss, ss.ss_len)
> == 0) {
> + log_msg(2, "config: skip duplicate peer %s", peer_addr);
> + return 0;
> + }
> + }
> +
> + return 1;
> }
>
> int
> Index: net.c
> ===
> RCS file: /cvs/src/usr.sbin/sasyncd/net.c,v
> retrieving revision 1.23
> diff -u -p -r1.23 net.c
> --- net.c 12 Dec 2015 20:04:23 - 1.23
> +++ net.c 26 Mar 2019 14:51:52 -
> @@ -71,7 +71,6 @@ AES_KEY aes_key[2];
>
> /* Local prototypes. */
> static u_int8_t *net_read(struct syncpeer *, u_int32_t *, u_int32_t *);
> -static intnet
Re: [patch] Re: Possible sasyncd memory leak ?
W dniu 25.03.2019 o 15:08, Otto Moerbeek pisze:
On Sat, Mar 23, 2019 at 06:07:02PM +0100, Michał Koc wrote:
... [snip]
This is almost good. You might fold host_ip() into net_set_sa(). the
double malloc and copy isn't really needed.
-Otto
Done, patch follows
Best regards
M.K
Index: conf.y
===
RCS file: /cvs/src/usr.sbin/sasyncd/conf.y,v
retrieving revision 1.19
diff -u -p -r1.19 conf.y
--- conf.y 9 Apr 2017 02:40:24 - 1.19
+++ conf.y 26 Mar 2019 14:51:52 -
@@ -32,6 +32,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -48,6 +49,7 @@ struct cfgstate cfgstate;
intconflen = 0;
char *confbuf, *confptr;
+intcheck_peer_addr(const char *);
intyyparse(void);
intyylex(void);
void yyerror(const char *);
@@ -172,29 +174,21 @@ setting : INTERFACE STRING
| PEER STRING
{
struct syncpeer *peer;
- int duplicate = 0;
- for (peer = LIST_FIRST(&cfgstate.peerlist); peer;
-peer = LIST_NEXT(peer, link))
- if (strcmp($2, peer->name) == 0) {
- duplicate++;
- break;
- }
- if (duplicate)
- free($2);
- else {
+ if (check_peer_addr($2)) {
peer = calloc(1, sizeof *peer);
- if (!peer) {
+ if (peer == NULL) {
log_err("config: calloc(1, %lu) "
"failed", sizeof *peer);
free($2);
YYERROR;
}
peer->name = $2;
- }
- LIST_INSERT_HEAD(&cfgstate.peerlist, peer, link);
- cfgstate.peercnt++;
- log_msg(2, "config: add peer %s", peer->name);
+ LIST_INSERT_HEAD(&cfgstate.peerlist, peer,
link);
+ cfgstate.peercnt++;
+ log_msg(2, "config: add peer %s", peer->name);
+ } else
+ free($2);
}
| LISTEN ON STRING af port
{
@@ -281,6 +275,46 @@ match(char *token)
sizeof keywords[0], match_cmp);
return k ? k->value : STRING;
+}
+
+int
+check_peer_addr(const char *peer_addr)
+{
+ struct ifaddrs *ifap = 0, *ifa;
+ struct syncpeer *peer;
+ struct sockaddr_storage ss, peer_ss;
+
+ if(net_set_sa((struct sockaddr *)&ss, peer_addr, 0) == -1) {
+ log_msg(2, "config: skip unparseable peer %s", peer_addr);
+ return 0;
+ }
+
+ if (getifaddrs(&ifap) == 0) {
+ for (ifa = ifap; ifa != NULL; ifa = ifa->ifa_next) {
+ if (ifa->ifa_addr == NULL || ifa->ifa_addr->sa_family
!= ss.ss_family)
+ continue;
+
+ if (ss.ss_len == ifa->ifa_addr->sa_len && memcmp(&ss,
ifa->ifa_addr, ss.ss_len) == 0) {
+ log_msg(2, "config: skip local peer %s",
peer_addr);
+ freeifaddrs(ifap);
+ return 0;
+ }
+ }
+ freeifaddrs(ifap);
+ }
+
+ for (peer = LIST_FIRST(&cfgstate.peerlist); peer != NULL; peer =
LIST_NEXT(peer, link)) {
+ if(net_set_sa((struct sockaddr *)&peer_ss, peer->name, 0) == -1) {
+ log_msg(2, "config: net_set_sa(%s) failed", peer->name);
+ continue;
+ }
+ if (ss.ss_len == peer_ss.ss_len && memcmp(&ss, &peer_ss, ss.ss_len)
== 0) {
+ log_msg(2, "config: skip duplicate peer %s", peer_addr);
+ return 0;
+ }
+ }
+
+ return 1;
}
int
Index: net.c
===
RCS file: /cvs/src/usr.sbin/sasyncd/net.c,v
retrieving revision 1.23
diff -u -p -r1.23 net.c
--- net.c 12 Dec 2015 20:04:23 - 1.23
+++ net.c 26 Mar 2019 14:51:52 -
@@ -71,7 +71,6 @@ AES_KEY aes_key[2];
/* Local prototypes. */
static u_int8_t *net_read(struct syncpeer *, u_int32_t *, u_int32_t *);
-static int net_set_sa(struct sockaddr *, char *, in_port_t);
static void net_check_peers(void *);
/* Pretty-print a buffer. */
@@ -752,35 +751,30 @@ net_read(struct syncpeer *p, u_int32_t *
return msg;
}
-static int
-net_set_sa(struct sockaddr *sa,
Re: [patch] Re: Possible sasyncd memory leak ?
On Sat, Mar 23, 2019 at 06:07:02PM +0100, Michał Koc wrote: > ... [snip] This is almost good. You might fold host_ip() into net_set_sa(). the double malloc and copy isn't really needed. -Otto
Re: [patch] Re: Possible sasyncd memory leak ?
On Sat, Mar 23, 2019 at 06:07:02PM +0100, Michał Koc wrote:
> W dniu 23.03.2019 o 10:09, Otto Moerbeek pisze:
> > On Fri, Mar 22, 2019 at 09:57:29PM +0100, Michał Koc wrote:
> >
> > > W dniu 22.03.2019 o 11:19, Michał Koc pisze:
> > > > W dniu 21.03.2019 o 11:52, Otto Moerbeek pisze:
> > > > > On Thu, Mar 21, 2019 at 09:28:28AM +0100, Michał Koc wrote:
> > > > >
> > > > > > W dniu 21.03.2019 o 07:21, Otto Moerbeek pisze:
> > > > > > > On Thu, Mar 21, 2019 at 12:51:11AM +0100, Klemens Nanni wrote:
> > > > > > > > On Tue, Mar 12, 2019 at 03:19:56PM +0100, Otto Moerbeek wrote:
> > > > > > > > > > I also fixed a case of parsing IPv6 addresses.
> > > > > > > > > >
> > > > > > > > > > Anyone willing to ok?
> > > > > > > > See comments inline.
> > > > > > > >
> > > > > > > > > And now also with a lexer bug fixed. Earlier I
> > > > > > > > > thougt it was an order
> > > > > > > > > dependency in the clauses. But is was an order dependency in
> > > > > > > > > comment
> > > > > > > > > lines and empty lines.
> > > > > > > > > +check_peer_addr(const char *peer_addr)
> > > > > > > > > +{
> > > > > > > > > + int valid = 1;
> > > > > > > > > + short peer_family = AF_UNSPEC;
> > > > > > > > > + struct ifaddrs *ifap = NULL, *ifa;
> > > > > > > > > + struct syncpeer *peer;
> > > > > > > > > + struct sockaddr_in sa;
> > > > > > > > > + struct sockaddr_in6 sa6;
> > > > > > > > > +
> > > > > > > > > + if (inet_pton(AF_INET, peer_addr, &sa.sin_addr) == 1)
> > > > > > > > > + peer_family = AF_INET;
> > > > > > > > > +
> > > > > > > > > + if (peer_family == AF_UNSPEC && inet_pton(AF_INET6,
> > > > > > > > > peer_addr,
> > > > > > > > > + &sa6.sin6_addr) == 1)
> > > > > > > > > + peer_family = AF_INET6;
> > > > > > > > `peer_addr' must not be a CIDR network, so I'd go with the more
> > > > > > > > AF
> > > > > > > > agnostic getaddrinfo(3) and check for res->ai_family in any
> > > > > > > > case...
> > > > > > > >
> > > > > > > > > + if (peer_family == AF_UNSPEC) {
> > > > > > > > > + log_msg(2, "config: skip unparseable peer %s",
> > > > > > > > > peer_addr);
> > > > > > > > > + valid = 0;
> > > > > > > > > + }
> > > > > > > > ..but `peer_addr' may also be a hostname, so that is not
> > > > > > > > handled by
> > > > > > > > your diff:
> > > > > > > >
> > > > > > > > net.h: char *name; /* FQDN or an IP, from
> > > > > > > > conf */
> > > > > > > >
> > > > > > > > getaddrinfo(3) can resolve however, thus inet_pton(3)
> > > > > > > > should not be used
> > > > > > > > here.
> > > > > > > >
> > > > > > > > Either way, this is a job for host_ip() as found in pfctl or
> > > > > > > > bgpd.
> > > > > > > > Other daemons like iked still have host_v4() and
> > > > > > > > host_v6(). Parsers use
> > > > > > > > these functions to check for valid addresses, so I'd say
> > > > > > > > sasyncd should
> > > > > > > > be no exception and adopt the same approach.
> > > > > > > >
> > > > > > > > > @@ -325,7 +386,7 @@ yylex(void)
> > > > > > > > > /* Numerical token? */
> > > > > > > > > if (isdigit(*confptr)) {
> > > > > > > > > for (p = confptr; *p; p++)
> > > > > > > > > - if (*p == '.') /* IP address, or bad input */
> > > > > > > > > + if (*p == '.' || *p == ':') /* IP
> > > > > > > > > address, or bad input */
> > > > > > > > This fixes the parser as in
> > > > > > > >
> > > > > > > > # echo peer 2001:db8::1 > conf
> > > > > > > > # sasyncd -dnv -c conf
> > > > > > > > config: syntax error
> > > > > > > > # ./obj/sasyncd -dnv -c conf
> > > > > > > > configuration OK
> > > > > > > >
> > > > > > > > But I have not actually tested this with a proper IPv6
> > > > > > > > setup since I do
> > > > > > > > not use sasyncd; did you?
> > > > > > > >
> > > > > > > > > @@ -397,6 +458,9 @@ conf_parse_file(char *cfgfile)
> > > > > > > > > if (*s == '#') {
> > > > > > > > > while (*s != '\n' && s < buf + conflen)
> > > > > > > > > s++;
> > > > > > > > > + while (*s == '\n' && s < buf + conflen)
> > > > > > > > > + s++;
> > > > > > > > > + s--;
> > > > > > > > OK kn for this fix alone.
> > > > > > > >
> > > > > > > I'll test an ipv6 seup and commit the lexer parts if those work.
> > > > > > > Michal, can you work on the first set of comments? I think
> > > > > > > Klemens is
> > > > > > > right.
> > > > > > >
> > > > > > > -Otto
> > > > > > >
> > > > > > Of course, I'll follow the comments soon
> > > > > >
> > > > > > Best Regards
> > > > > > M.K.
> > > > the diff is ready, what happened here:
> > > >
> > > > 1. host => ip manipulation moved to host_ip() function in net.c and is
> > > > using getaddrinfo()
> > > > 2. net_set_sa() in net.c modified to use host_ip()
> > > >
> > > > Bets Regards
> > > > M.K
> > >
> All comments applied.
> 1. All pointers in the patch are matched
Re: [patch] Re: Possible sasyncd memory leak ?
W dniu 23.03.2019 o 10:09, Otto Moerbeek pisze:
On Fri, Mar 22, 2019 at 09:57:29PM +0100, Michał Koc wrote:
W dniu 22.03.2019 o 11:19, Michał Koc pisze:
W dniu 21.03.2019 o 11:52, Otto Moerbeek pisze:
On Thu, Mar 21, 2019 at 09:28:28AM +0100, Michał Koc wrote:
W dniu 21.03.2019 o 07:21, Otto Moerbeek pisze:
On Thu, Mar 21, 2019 at 12:51:11AM +0100, Klemens Nanni wrote:
On Tue, Mar 12, 2019 at 03:19:56PM +0100, Otto Moerbeek wrote:
I also fixed a case of parsing IPv6 addresses.
Anyone willing to ok?
See comments inline.
And now also with a lexer bug fixed. Earlier I
thougt it was an order
dependency in the clauses. But is was an order dependency in comment
lines and empty lines.
+check_peer_addr(const char *peer_addr)
+{
+ int valid = 1;
+ short peer_family = AF_UNSPEC;
+ struct ifaddrs *ifap = NULL, *ifa;
+ struct syncpeer *peer;
+ struct sockaddr_in sa;
+ struct sockaddr_in6 sa6;
+
+ if (inet_pton(AF_INET, peer_addr, &sa.sin_addr) == 1)
+ peer_family = AF_INET;
+
+ if (peer_family == AF_UNSPEC && inet_pton(AF_INET6, peer_addr,
+ &sa6.sin6_addr) == 1)
+ peer_family = AF_INET6;
`peer_addr' must not be a CIDR network, so I'd go with the more AF
agnostic getaddrinfo(3) and check for res->ai_family in any case...
+ if (peer_family == AF_UNSPEC) {
+ log_msg(2, "config: skip unparseable peer %s", peer_addr);
+ valid = 0;
+ }
..but `peer_addr' may also be a hostname, so that is not handled by
your diff:
net.h: char *name; /* FQDN or an IP, from conf */
getaddrinfo(3) can resolve however, thus inet_pton(3)
should not be used
here.
Either way, this is a job for host_ip() as found in pfctl or bgpd.
Other daemons like iked still have host_v4() and
host_v6(). Parsers use
these functions to check for valid addresses, so I'd say
sasyncd should
be no exception and adopt the same approach.
@@ -325,7 +386,7 @@ yylex(void)
/* Numerical token? */
if (isdigit(*confptr)) {
for (p = confptr; *p; p++)
- if (*p == '.') /* IP address, or bad input */
+ if (*p == '.' || *p == ':') /* IP
address, or bad input */
This fixes the parser as in
# echo peer 2001:db8::1 > conf
# sasyncd -dnv -c conf
config: syntax error
# ./obj/sasyncd -dnv -c conf
configuration OK
But I have not actually tested this with a proper IPv6
setup since I do
not use sasyncd; did you?
@@ -397,6 +458,9 @@ conf_parse_file(char *cfgfile)
if (*s == '#') {
while (*s != '\n' && s < buf + conflen)
s++;
+ while (*s == '\n' && s < buf + conflen)
+ s++;
+ s--;
OK kn for this fix alone.
I'll test an ipv6 seup and commit the lexer parts if those work.
Michal, can you work on the first set of comments? I think Klemens is
right.
-Otto
Of course, I'll follow the comments soon
Best Regards
M.K.
the diff is ready, what happened here:
1. host => ip manipulation moved to host_ip() function in net.c and is
using getaddrinfo()
2. net_set_sa() in net.c modified to use host_ip()
Bets Regards
M.K
All comments applied.
1. All pointers in the patch are matched against NULL. But there are
tons of pointers matched to 0 or unary operator "!" in the source code,
should all of them be changed ?
2. Memory allocation repaired
Best regards
M.K.
Index: conf.y
===
RCS file: /cvs/src/usr.sbin/sasyncd/conf.y,v
retrieving revision 1.19
diff -u -p -r1.19 conf.y
--- conf.y 9 Apr 2017 02:40:24 - 1.19
+++ conf.y 23 Mar 2019 16:59:03 -
@@ -30,8 +30,10 @@
#include
#include
#include
+#include
#include
#include
+#include
#include
#include
#include
@@ -48,6 +50,7 @@ struct cfgstate cfgstate;
intconflen = 0;
char *confbuf, *confptr;
+intcheck_peer_addr(const char *);
intyyparse(void);
intyylex(void);
void yyerror(const char *);
@@ -172,29 +175,21 @@ setting : INTERFACE STRING
| PEER STRING
{
struct syncpeer *peer;
- int duplicate = 0;
- for (peer = LIST_FIRST(&cfgstate.peerlist); peer;
-peer = LIST_NEXT(peer, link))
- if (strcmp($2, peer->name) == 0) {
- duplicate++;
- break;
- }
- if (duplicate)
- free($2);
- else {
+ if (check_peer_addr($2)) {
peer = calloc(1, sizeof *peer);
- if (!peer) {
+ if (peer == NULL) {
log_err("config: calloc(1, %lu) "
Re: [patch] Re: Possible sasyncd memory leak ?
On Fri, Mar 22, 2019 at 09:57:29PM +0100, Michał Koc wrote:
> W dniu 22.03.2019 o 11:19, Michał Koc pisze:
> > W dniu 21.03.2019 o 11:52, Otto Moerbeek pisze:
> > > On Thu, Mar 21, 2019 at 09:28:28AM +0100, Michał Koc wrote:
> > >
> > > > W dniu 21.03.2019 o 07:21, Otto Moerbeek pisze:
> > > > > On Thu, Mar 21, 2019 at 12:51:11AM +0100, Klemens Nanni wrote:
> > > > > > On Tue, Mar 12, 2019 at 03:19:56PM +0100, Otto Moerbeek wrote:
> > > > > > > > I also fixed a case of parsing IPv6 addresses.
> > > > > > > >
> > > > > > > > Anyone willing to ok?
> > > > > > See comments inline.
> > > > > >
> > > > > > > And now also with a lexer bug fixed. Earlier I
> > > > > > > thougt it was an order
> > > > > > > dependency in the clauses. But is was an order dependency in
> > > > > > > comment
> > > > > > > lines and empty lines.
> > > > > > > +check_peer_addr(const char *peer_addr)
> > > > > > > +{
> > > > > > > + int valid = 1;
> > > > > > > + short peer_family = AF_UNSPEC;
> > > > > > > + struct ifaddrs *ifap = NULL, *ifa;
> > > > > > > + struct syncpeer *peer;
> > > > > > > + struct sockaddr_in sa;
> > > > > > > + struct sockaddr_in6 sa6;
> > > > > > > +
> > > > > > > + if (inet_pton(AF_INET, peer_addr, &sa.sin_addr) == 1)
> > > > > > > + peer_family = AF_INET;
> > > > > > > +
> > > > > > > + if (peer_family == AF_UNSPEC && inet_pton(AF_INET6,
> > > > > > > peer_addr,
> > > > > > > + &sa6.sin6_addr) == 1)
> > > > > > > + peer_family = AF_INET6;
> > > > > > `peer_addr' must not be a CIDR network, so I'd go with the more AF
> > > > > > agnostic getaddrinfo(3) and check for res->ai_family in any case...
> > > > > >
> > > > > > > + if (peer_family == AF_UNSPEC) {
> > > > > > > + log_msg(2, "config: skip unparseable peer %s",
> > > > > > > peer_addr);
> > > > > > > + valid = 0;
> > > > > > > + }
> > > > > > ..but `peer_addr' may also be a hostname, so that is not handled by
> > > > > > your diff:
> > > > > >
> > > > > > net.h: char *name; /* FQDN or an IP, from conf
> > > > > > */
> > > > > >
> > > > > > getaddrinfo(3) can resolve however, thus inet_pton(3)
> > > > > > should not be used
> > > > > > here.
> > > > > >
> > > > > > Either way, this is a job for host_ip() as found in pfctl or bgpd.
> > > > > > Other daemons like iked still have host_v4() and
> > > > > > host_v6(). Parsers use
> > > > > > these functions to check for valid addresses, so I'd say
> > > > > > sasyncd should
> > > > > > be no exception and adopt the same approach.
> > > > > >
> > > > > > > @@ -325,7 +386,7 @@ yylex(void)
> > > > > > > /* Numerical token? */
> > > > > > > if (isdigit(*confptr)) {
> > > > > > > for (p = confptr; *p; p++)
> > > > > > > - if (*p == '.') /* IP address, or bad input */
> > > > > > > + if (*p == '.' || *p == ':') /* IP
> > > > > > > address, or bad input */
> > > > > > This fixes the parser as in
> > > > > >
> > > > > > # echo peer 2001:db8::1 > conf
> > > > > > # sasyncd -dnv -c conf
> > > > > > config: syntax error
> > > > > > # ./obj/sasyncd -dnv -c conf
> > > > > > configuration OK
> > > > > >
> > > > > > But I have not actually tested this with a proper IPv6
> > > > > > setup since I do
> > > > > > not use sasyncd; did you?
> > > > > >
> > > > > > > @@ -397,6 +458,9 @@ conf_parse_file(char *cfgfile)
> > > > > > > if (*s == '#') {
> > > > > > > while (*s != '\n' && s < buf + conflen)
> > > > > > > s++;
> > > > > > > + while (*s == '\n' && s < buf + conflen)
> > > > > > > + s++;
> > > > > > > + s--;
> > > > > > OK kn for this fix alone.
> > > > > >
> > > > > I'll test an ipv6 seup and commit the lexer parts if those work.
> > > > > Michal, can you work on the first set of comments? I think Klemens is
> > > > > right.
> > > > >
> > > > > -Otto
> > > > >
> > > > Of course, I'll follow the comments soon
> > > >
> > > > Best Regards
> > > > M.K.
> >
> > the diff is ready, what happened here:
> >
> > 1. host => ip manipulation moved to host_ip() function in net.c and is
> > using getaddrinfo()
> > 2. net_set_sa() in net.c modified to use host_ip()
> >
> > Bets Regards
> > M.K
>
> Do not actually compare two structs sockaddr if their lengths differ.
>
I did not have time yet to run this code, but some comments inline.
-Otto
> Index: conf.y
> ===
> RCS file: /cvs/src/usr.sbin/sasyncd/conf.y,v
> retrieving revision 1.19
> diff -u -p -r1.19 conf.y
> --- conf.y9 Apr 2017 02:40:24 - 1.19
> +++ conf.y22 Mar 2019 20:55:21 -
> @@ -30,8 +30,10 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -48,6 +50,7 @@ struct cfgstate cfgstate;
> int conflen = 0;
> char *confbuf, *confptr;
>
Re: [patch] Re: Possible sasyncd memory leak ?
W dniu 22.03.2019 o 11:19, Michał Koc pisze:
W dniu 21.03.2019 o 11:52, Otto Moerbeek pisze:
On Thu, Mar 21, 2019 at 09:28:28AM +0100, Michał Koc wrote:
W dniu 21.03.2019 o 07:21, Otto Moerbeek pisze:
On Thu, Mar 21, 2019 at 12:51:11AM +0100, Klemens Nanni wrote:
On Tue, Mar 12, 2019 at 03:19:56PM +0100, Otto Moerbeek wrote:
I also fixed a case of parsing IPv6 addresses.
Anyone willing to ok?
See comments inline.
And now also with a lexer bug fixed. Earlier I thougt it was an
order
dependency in the clauses. But is was an order dependency in comment
lines and empty lines.
+check_peer_addr(const char *peer_addr)
+{
+ int valid = 1;
+ short peer_family = AF_UNSPEC;
+ struct ifaddrs *ifap = NULL, *ifa;
+ struct syncpeer *peer;
+ struct sockaddr_in sa;
+ struct sockaddr_in6 sa6;
+
+ if (inet_pton(AF_INET, peer_addr, &sa.sin_addr) == 1)
+ peer_family = AF_INET;
+
+ if (peer_family == AF_UNSPEC && inet_pton(AF_INET6, peer_addr,
+ &sa6.sin6_addr) == 1)
+ peer_family = AF_INET6;
`peer_addr' must not be a CIDR network, so I'd go with the more AF
agnostic getaddrinfo(3) and check for res->ai_family in any case...
+ if (peer_family == AF_UNSPEC) {
+ log_msg(2, "config: skip unparseable peer %s", peer_addr);
+ valid = 0;
+ }
..but `peer_addr' may also be a hostname, so that is not handled by
your diff:
net.h: char *name; /* FQDN or an IP, from conf */
getaddrinfo(3) can resolve however, thus inet_pton(3) should not
be used
here.
Either way, this is a job for host_ip() as found in pfctl or bgpd.
Other daemons like iked still have host_v4() and host_v6().
Parsers use
these functions to check for valid addresses, so I'd say sasyncd
should
be no exception and adopt the same approach.
@@ -325,7 +386,7 @@ yylex(void)
/* Numerical token? */
if (isdigit(*confptr)) {
for (p = confptr; *p; p++)
- if (*p == '.') /* IP address, or bad input */
+ if (*p == '.' || *p == ':') /* IP address, or bad
input */
This fixes the parser as in
# echo peer 2001:db8::1 > conf
# sasyncd -dnv -c conf
config: syntax error
# ./obj/sasyncd -dnv -c conf
configuration OK
But I have not actually tested this with a proper IPv6 setup since
I do
not use sasyncd; did you?
@@ -397,6 +458,9 @@ conf_parse_file(char *cfgfile)
if (*s == '#') {
while (*s != '\n' && s < buf + conflen)
s++;
+ while (*s == '\n' && s < buf + conflen)
+ s++;
+ s--;
OK kn for this fix alone.
I'll test an ipv6 seup and commit the lexer parts if those work.
Michal, can you work on the first set of comments? I think Klemens is
right.
-Otto
Of course, I'll follow the comments soon
Best Regards
M.K.
the diff is ready, what happened here:
1. host => ip manipulation moved to host_ip() function in net.c and is
using getaddrinfo()
2. net_set_sa() in net.c modified to use host_ip()
Bets Regards
M.K
Do not actually compare two structs sockaddr if their lengths differ.
Index: conf.y
===
RCS file: /cvs/src/usr.sbin/sasyncd/conf.y,v
retrieving revision 1.19
diff -u -p -r1.19 conf.y
--- conf.y 9 Apr 2017 02:40:24 - 1.19
+++ conf.y 22 Mar 2019 20:55:21 -
@@ -30,8 +30,10 @@
#include
#include
#include
+#include
#include
#include
+#include
#include
#include
#include
@@ -48,6 +50,7 @@ struct cfgstate cfgstate;
intconflen = 0;
char *confbuf, *confptr;
+intcheck_peer_addr(const char *);
intyyparse(void);
intyylex(void);
void yyerror(const char *);
@@ -172,17 +175,8 @@ setting: INTERFACE STRING
| PEER STRING
{
struct syncpeer *peer;
- int duplicate = 0;
- for (peer = LIST_FIRST(&cfgstate.peerlist); peer;
-peer = LIST_NEXT(peer, link))
- if (strcmp($2, peer->name) == 0) {
- duplicate++;
- break;
- }
- if (duplicate)
- free($2);
- else {
+ if (check_peer_addr($2)) {
peer = calloc(1, sizeof *peer);
if (!peer) {
log_err("config: calloc(1, %lu) "
@@ -191,10 +185,11 @@ setting : INTERFACE STRING
YYERROR;
}
peer->name = $2;
- }
- LIST_INSERT_HEAD(&cfgstate.peerlist, peer, link);
- cfgstate.peercnt++;
- log_
Re: [patch] Re: Possible sasyncd memory leak ?
W dniu 22.03.2019 o 11:19, Michał Koc pisze:
W dniu 21.03.2019 o 11:52, Otto Moerbeek pisze:
On Thu, Mar 21, 2019 at 09:28:28AM +0100, Michał Koc wrote:
W dniu 21.03.2019 o 07:21, Otto Moerbeek pisze:
On Thu, Mar 21, 2019 at 12:51:11AM +0100, Klemens Nanni wrote:
On Tue, Mar 12, 2019 at 03:19:56PM +0100, Otto Moerbeek wrote:
I also fixed a case of parsing IPv6 addresses.
Anyone willing to ok?
See comments inline.
And now also with a lexer bug fixed. Earlier I thougt it was an
order
dependency in the clauses. But is was an order dependency in comment
lines and empty lines.
+check_peer_addr(const char *peer_addr)
+{
+ int valid = 1;
+ short peer_family = AF_UNSPEC;
+ struct ifaddrs *ifap = NULL, *ifa;
+ struct syncpeer *peer;
+ struct sockaddr_in sa;
+ struct sockaddr_in6 sa6;
+
+ if (inet_pton(AF_INET, peer_addr, &sa.sin_addr) == 1)
+ peer_family = AF_INET;
+
+ if (peer_family == AF_UNSPEC && inet_pton(AF_INET6, peer_addr,
+ &sa6.sin6_addr) == 1)
+ peer_family = AF_INET6;
`peer_addr' must not be a CIDR network, so I'd go with the more AF
agnostic getaddrinfo(3) and check for res->ai_family in any case...
+ if (peer_family == AF_UNSPEC) {
+ log_msg(2, "config: skip unparseable peer %s", peer_addr);
+ valid = 0;
+ }
..but `peer_addr' may also be a hostname, so that is not handled by
your diff:
net.h: char *name; /* FQDN or an IP, from
conf */
getaddrinfo(3) can resolve however, thus inet_pton(3) should not
be used
here.
Either way, this is a job for host_ip() as found in pfctl or bgpd.
Other daemons like iked still have host_v4() and host_v6().
Parsers use
these functions to check for valid addresses, so I'd say sasyncd
should
be no exception and adopt the same approach.
@@ -325,7 +386,7 @@ yylex(void)
/* Numerical token? */
if (isdigit(*confptr)) {
for (p = confptr; *p; p++)
- if (*p == '.') /* IP address, or bad input */
+ if (*p == '.' || *p == ':') /* IP address, or bad
input */
This fixes the parser as in
# echo peer 2001:db8::1 > conf
# sasyncd -dnv -c conf
config: syntax error
# ./obj/sasyncd -dnv -c conf
configuration OK
But I have not actually tested this with a proper IPv6 setup since
I do
not use sasyncd; did you?
@@ -397,6 +458,9 @@ conf_parse_file(char *cfgfile)
if (*s == '#') {
while (*s != '\n' && s < buf + conflen)
s++;
+ while (*s == '\n' && s < buf + conflen)
+ s++;
+ s--;
OK kn for this fix alone.
I'll test an ipv6 seup and commit the lexer parts if those work.
Michal, can you work on the first set of comments? I think Klemens is
right.
-Otto
Of course, I'll follow the comments soon
Best Regards
M.K.
the diff is ready, what happened here:
1. host => ip manipulation moved to host_ip() function in net.c and is
using getaddrinfo()
2. net_set_sa() in net.c modified to use host_ip()
Bets Regards
M.K
Additional range check when comparing memory contents
Index: conf.y
===
RCS file: /cvs/src/usr.sbin/sasyncd/conf.y,v
retrieving revision 1.19
diff -u -p -r1.19 conf.y
--- conf.y 9 Apr 2017 02:40:24 - 1.19
+++ conf.y 22 Mar 2019 14:27:49 -
@@ -30,8 +30,10 @@
#include
#include
#include
+#include
#include
#include
+#include
#include
#include
#include
@@ -48,6 +50,7 @@ struct cfgstate cfgstate;
intconflen = 0;
char *confbuf, *confptr;
+intcheck_peer_addr(const char *);
intyyparse(void);
intyylex(void);
void yyerror(const char *);
@@ -172,17 +175,8 @@ setting: INTERFACE STRING
| PEER STRING
{
struct syncpeer *peer;
- int duplicate = 0;
- for (peer = LIST_FIRST(&cfgstate.peerlist); peer;
-peer = LIST_NEXT(peer, link))
- if (strcmp($2, peer->name) == 0) {
- duplicate++;
- break;
- }
- if (duplicate)
- free($2);
- else {
+ if (check_peer_addr($2)) {
peer = calloc(1, sizeof *peer);
if (!peer) {
log_err("config: calloc(1, %lu) "
@@ -191,10 +185,11 @@ setting : INTERFACE STRING
YYERROR;
}
peer->name = $2;
- }
- LIST_INSERT_HEAD(&cfgstate.peerlist, peer, link);
- cfgstate.peercnt++;
- log_msg(2, "config: ad
Re: [patch] Re: Possible sasyncd memory leak ?
W dniu 21.03.2019 o 11:52, Otto Moerbeek pisze:
On Thu, Mar 21, 2019 at 09:28:28AM +0100, Michał Koc wrote:
W dniu 21.03.2019 o 07:21, Otto Moerbeek pisze:
On Thu, Mar 21, 2019 at 12:51:11AM +0100, Klemens Nanni wrote:
On Tue, Mar 12, 2019 at 03:19:56PM +0100, Otto Moerbeek wrote:
I also fixed a case of parsing IPv6 addresses.
Anyone willing to ok?
See comments inline.
And now also with a lexer bug fixed. Earlier I thougt it was an order
dependency in the clauses. But is was an order dependency in comment
lines and empty lines.
+check_peer_addr(const char *peer_addr)
+{
+ int valid = 1;
+ short peer_family = AF_UNSPEC;
+ struct ifaddrs *ifap = NULL, *ifa;
+ struct syncpeer *peer;
+ struct sockaddr_in sa;
+ struct sockaddr_in6 sa6;
+
+ if (inet_pton(AF_INET, peer_addr, &sa.sin_addr) == 1)
+ peer_family = AF_INET;
+
+ if (peer_family == AF_UNSPEC && inet_pton(AF_INET6, peer_addr,
+ &sa6.sin6_addr) == 1)
+ peer_family = AF_INET6;
`peer_addr' must not be a CIDR network, so I'd go with the more AF
agnostic getaddrinfo(3) and check for res->ai_family in any case...
+ if (peer_family == AF_UNSPEC) {
+ log_msg(2, "config: skip unparseable peer %s", peer_addr);
+ valid = 0;
+ }
..but `peer_addr' may also be a hostname, so that is not handled by
your diff:
net.h: char*name; /* FQDN or an IP, from conf */
getaddrinfo(3) can resolve however, thus inet_pton(3) should not be used
here.
Either way, this is a job for host_ip() as found in pfctl or bgpd.
Other daemons like iked still have host_v4() and host_v6(). Parsers use
these functions to check for valid addresses, so I'd say sasyncd should
be no exception and adopt the same approach.
@@ -325,7 +386,7 @@ yylex(void)
/* Numerical token? */
if (isdigit(*confptr)) {
for (p = confptr; *p; p++)
- if (*p == '.') /* IP address, or bad input */
+ if (*p == '.' || *p == ':') /* IP address, or bad input
*/
This fixes the parser as in
# echo peer 2001:db8::1 > conf
# sasyncd -dnv -c conf
config: syntax error
# ./obj/sasyncd -dnv -c conf
configuration OK
But I have not actually tested this with a proper IPv6 setup since I do
not use sasyncd; did you?
@@ -397,6 +458,9 @@ conf_parse_file(char *cfgfile)
if (*s == '#') {
while (*s != '\n' && s < buf + conflen)
s++;
+ while (*s == '\n' && s < buf + conflen)
+ s++;
+ s--;
OK kn for this fix alone.
I'll test an ipv6 seup and commit the lexer parts if those work.
Michal, can you work on the first set of comments? I think Klemens is
right.
-Otto
Of course, I'll follow the comments soon
Best Regards
M.K.
the diff is ready, what happened here:
1. host => ip manipulation moved to host_ip() function in net.c and is
using getaddrinfo()
2. net_set_sa() in net.c modified to use host_ip()
Bets Regards
M.K.
Index: conf.y
===
RCS file: /cvs/src/usr.sbin/sasyncd/conf.y,v
retrieving revision 1.19
diff -u -p -r1.19 conf.y
--- conf.y 9 Apr 2017 02:40:24 - 1.19
+++ conf.y 22 Mar 2019 09:16:37 -
@@ -30,8 +30,10 @@
#include
#include
#include
+#include
#include
#include
+#include
#include
#include
#include
@@ -48,6 +50,7 @@ struct cfgstate cfgstate;
intconflen = 0;
char *confbuf, *confptr;
+intcheck_peer_addr(const char *);
intyyparse(void);
intyylex(void);
void yyerror(const char *);
@@ -172,17 +175,8 @@ setting: INTERFACE STRING
| PEER STRING
{
struct syncpeer *peer;
- int duplicate = 0;
- for (peer = LIST_FIRST(&cfgstate.peerlist); peer;
-peer = LIST_NEXT(peer, link))
- if (strcmp($2, peer->name) == 0) {
- duplicate++;
- break;
- }
- if (duplicate)
- free($2);
- else {
+ if (check_peer_addr($2)) {
peer = calloc(1, sizeof *peer);
if (!peer) {
log_err("config: calloc(1, %lu) "
@@ -191,10 +185,11 @@ setting : INTERFACE STRING
YYERROR;
}
peer->name = $2;
- }
- LIST_INSERT_HEAD(&cfgstate.peerlist, peer, link);
-
Re: [patch] Re: Possible sasyncd memory leak ?
Otto Moerbeek wrote:
This lex operation seems ridiculous.
Normal one does lex of free-standing digit sequences into a "number
token" only if the sequence is fully digits -- see the code around
pfctl/parse.y line 5368
Or don't lex numbers instead, but consider them strings (with the correct
seperators), and do the conversion higher in the parse.
What's going on here has been solved in numerous other domain-specific
parse.y adapted originally from pfctl into 30 other parse.y. I was
unaware that sasyncd has a unique parser...
Just commenting; it would be more than a few hours to bring in parse.y
> Meanwhile, I tested a IPv6 setup, it works ok.
> So I'm going to commit the diff below,
>
> -Otto
>
> Index: conf.y
> ===
> RCS file: /cvs/src/usr.sbin/sasyncd/conf.y,v
> retrieving revision 1.19
> diff -u -p -r1.19 conf.y
> --- conf.y9 Apr 2017 02:40:24 - 1.19
> +++ conf.y21 Mar 2019 10:51:46 -
> @@ -325,7 +325,7 @@ yylex(void)
> /* Numerical token? */
> if (isdigit(*confptr)) {
> for (p = confptr; *p; p++)
> - if (*p == '.') /* IP address, or bad input */
> + if (*p == '.' || *p == ':') /* IP address, or bad input
> */
> goto is_string;
> v = (int)strtol(confptr, (char **)NULL, 10);
> yylval.val = v;
> @@ -397,6 +397,9 @@ conf_parse_file(char *cfgfile)
> if (*s == '#') {
> while (*s != '\n' && s < buf + conflen)
> s++;
> + while (*s == '\n' && s < buf + conflen)
> + s++;
> + s--;
> continue;
> }
> if (d == buf && isspace(*s))
>
Re: [patch] Re: Possible sasyncd memory leak ?
On Thu, Mar 21, 2019 at 11:52:56AM +0100, Otto Moerbeek wrote: > Meanwhile, I tested a IPv6 setup, it works ok. > So I'm going to commit the diff below, Thanks! OK kn
Re: [patch] Re: Possible sasyncd memory leak ?
On Thu, Mar 21, 2019 at 09:28:28AM +0100, Michał Koc wrote:
> W dniu 21.03.2019 o 07:21, Otto Moerbeek pisze:
> > On Thu, Mar 21, 2019 at 12:51:11AM +0100, Klemens Nanni wrote:
> > > On Tue, Mar 12, 2019 at 03:19:56PM +0100, Otto Moerbeek wrote:
> > > > > I also fixed a case of parsing IPv6 addresses.
> > > > >
> > > > > Anyone willing to ok?
> > > See comments inline.
> > >
> > > > And now also with a lexer bug fixed. Earlier I thougt it was an order
> > > > dependency in the clauses. But is was an order dependency in comment
> > > > lines and empty lines.
> > > > +check_peer_addr(const char *peer_addr)
> > > > +{
> > > > + int valid = 1;
> > > > + short peer_family = AF_UNSPEC;
> > > > + struct ifaddrs *ifap = NULL, *ifa;
> > > > + struct syncpeer *peer;
> > > > + struct sockaddr_in sa;
> > > > + struct sockaddr_in6 sa6;
> > > > +
> > > > + if (inet_pton(AF_INET, peer_addr, &sa.sin_addr) == 1)
> > > > + peer_family = AF_INET;
> > > > +
> > > > + if (peer_family == AF_UNSPEC && inet_pton(AF_INET6, peer_addr,
> > > > + &sa6.sin6_addr) == 1)
> > > > + peer_family = AF_INET6;
> > > `peer_addr' must not be a CIDR network, so I'd go with the more AF
> > > agnostic getaddrinfo(3) and check for res->ai_family in any case...
> > >
> > > > + if (peer_family == AF_UNSPEC) {
> > > > + log_msg(2, "config: skip unparseable peer %s",
> > > > peer_addr);
> > > > + valid = 0;
> > > > + }
> > > ..but `peer_addr' may also be a hostname, so that is not handled by
> > > your diff:
> > >
> > > net.h: char*name; /* FQDN or an IP, from conf */
> > >
> > > getaddrinfo(3) can resolve however, thus inet_pton(3) should not be used
> > > here.
> > >
> > > Either way, this is a job for host_ip() as found in pfctl or bgpd.
> > > Other daemons like iked still have host_v4() and host_v6(). Parsers use
> > > these functions to check for valid addresses, so I'd say sasyncd should
> > > be no exception and adopt the same approach.
> > >
> > > > @@ -325,7 +386,7 @@ yylex(void)
> > > > /* Numerical token? */
> > > > if (isdigit(*confptr)) {
> > > > for (p = confptr; *p; p++)
> > > > - if (*p == '.') /* IP address, or bad input */
> > > > + if (*p == '.' || *p == ':') /* IP address, or
> > > > bad input */
> > > This fixes the parser as in
> > >
> > > # echo peer 2001:db8::1 > conf
> > > # sasyncd -dnv -c conf
> > > config: syntax error
> > > # ./obj/sasyncd -dnv -c conf
> > > configuration OK
> > >
> > > But I have not actually tested this with a proper IPv6 setup since I do
> > > not use sasyncd; did you?
> > >
> > > > @@ -397,6 +458,9 @@ conf_parse_file(char *cfgfile)
> > > > if (*s == '#') {
> > > > while (*s != '\n' && s < buf + conflen)
> > > > s++;
> > > > + while (*s == '\n' && s < buf + conflen)
> > > > + s++;
> > > > + s--;
> > > OK kn for this fix alone.
> > >
> > I'll test an ipv6 seup and commit the lexer parts if those work.
> > Michal, can you work on the first set of comments? I think Klemens is
> > right.
> >
> > -Otto
> >
> Of course, I'll follow the comments soon
>
> Best Regards
> M.K.
>
Meanwhile, I tested a IPv6 setup, it works ok.
So I'm going to commit the diff below,
-Otto
Index: conf.y
===
RCS file: /cvs/src/usr.sbin/sasyncd/conf.y,v
retrieving revision 1.19
diff -u -p -r1.19 conf.y
--- conf.y 9 Apr 2017 02:40:24 - 1.19
+++ conf.y 21 Mar 2019 10:51:46 -
@@ -325,7 +325,7 @@ yylex(void)
/* Numerical token? */
if (isdigit(*confptr)) {
for (p = confptr; *p; p++)
- if (*p == '.') /* IP address, or bad input */
+ if (*p == '.' || *p == ':') /* IP address, or bad input
*/
goto is_string;
v = (int)strtol(confptr, (char **)NULL, 10);
yylval.val = v;
@@ -397,6 +397,9 @@ conf_parse_file(char *cfgfile)
if (*s == '#') {
while (*s != '\n' && s < buf + conflen)
s++;
+ while (*s == '\n' && s < buf + conflen)
+ s++;
+ s--;
continue;
}
if (d == buf && isspace(*s))
Re: [patch] Re: Possible sasyncd memory leak ?
W dniu 21.03.2019 o 07:21, Otto Moerbeek pisze:
On Thu, Mar 21, 2019 at 12:51:11AM +0100, Klemens Nanni wrote:
On Tue, Mar 12, 2019 at 03:19:56PM +0100, Otto Moerbeek wrote:
I also fixed a case of parsing IPv6 addresses.
Anyone willing to ok?
See comments inline.
And now also with a lexer bug fixed. Earlier I thougt it was an order
dependency in the clauses. But is was an order dependency in comment
lines and empty lines.
+check_peer_addr(const char *peer_addr)
+{
+ int valid = 1;
+ short peer_family = AF_UNSPEC;
+ struct ifaddrs *ifap = NULL, *ifa;
+ struct syncpeer *peer;
+ struct sockaddr_in sa;
+ struct sockaddr_in6 sa6;
+
+ if (inet_pton(AF_INET, peer_addr, &sa.sin_addr) == 1)
+ peer_family = AF_INET;
+
+ if (peer_family == AF_UNSPEC && inet_pton(AF_INET6, peer_addr,
+ &sa6.sin6_addr) == 1)
+ peer_family = AF_INET6;
`peer_addr' must not be a CIDR network, so I'd go with the more AF
agnostic getaddrinfo(3) and check for res->ai_family in any case...
+ if (peer_family == AF_UNSPEC) {
+ log_msg(2, "config: skip unparseable peer %s", peer_addr);
+ valid = 0;
+ }
..but `peer_addr' may also be a hostname, so that is not handled by
your diff:
net.h: char*name; /* FQDN or an IP, from conf */
getaddrinfo(3) can resolve however, thus inet_pton(3) should not be used
here.
Either way, this is a job for host_ip() as found in pfctl or bgpd.
Other daemons like iked still have host_v4() and host_v6(). Parsers use
these functions to check for valid addresses, so I'd say sasyncd should
be no exception and adopt the same approach.
@@ -325,7 +386,7 @@ yylex(void)
/* Numerical token? */
if (isdigit(*confptr)) {
for (p = confptr; *p; p++)
- if (*p == '.') /* IP address, or bad input */
+ if (*p == '.' || *p == ':') /* IP address, or bad input
*/
This fixes the parser as in
# echo peer 2001:db8::1 > conf
# sasyncd -dnv -c conf
config: syntax error
# ./obj/sasyncd -dnv -c conf
configuration OK
But I have not actually tested this with a proper IPv6 setup since I do
not use sasyncd; did you?
@@ -397,6 +458,9 @@ conf_parse_file(char *cfgfile)
if (*s == '#') {
while (*s != '\n' && s < buf + conflen)
s++;
+ while (*s == '\n' && s < buf + conflen)
+ s++;
+ s--;
OK kn for this fix alone.
I'll test an ipv6 seup and commit the lexer parts if those work.
Michal, can you work on the first set of comments? I think Klemens is
right.
-Otto
Of course, I'll follow the comments soon
Best Regards
M.K.
Re: [patch] Re: Possible sasyncd memory leak ?
On Thu, Mar 21, 2019 at 12:51:11AM +0100, Klemens Nanni wrote:
>
> On Tue, Mar 12, 2019 at 03:19:56PM +0100, Otto Moerbeek wrote:
> > > I also fixed a case of parsing IPv6 addresses.
> > >
> > > Anyone willing to ok?
> See comments inline.
>
> > And now also with a lexer bug fixed. Earlier I thougt it was an order
> > dependency in the clauses. But is was an order dependency in comment
> > lines and empty lines.
>
> > +check_peer_addr(const char *peer_addr)
> > +{
> > + int valid = 1;
> > + short peer_family = AF_UNSPEC;
> > + struct ifaddrs *ifap = NULL, *ifa;
> > + struct syncpeer *peer;
> > + struct sockaddr_in sa;
> > + struct sockaddr_in6 sa6;
> > +
> > + if (inet_pton(AF_INET, peer_addr, &sa.sin_addr) == 1)
> > + peer_family = AF_INET;
> > +
> > + if (peer_family == AF_UNSPEC && inet_pton(AF_INET6, peer_addr,
> > + &sa6.sin6_addr) == 1)
> > + peer_family = AF_INET6;
> `peer_addr' must not be a CIDR network, so I'd go with the more AF
> agnostic getaddrinfo(3) and check for res->ai_family in any case...
>
> > + if (peer_family == AF_UNSPEC) {
> > + log_msg(2, "config: skip unparseable peer %s", peer_addr);
> > + valid = 0;
> > + }
> ..but `peer_addr' may also be a hostname, so that is not handled by
> your diff:
>
> net.h: char*name; /* FQDN or an IP, from conf */
>
> getaddrinfo(3) can resolve however, thus inet_pton(3) should not be used
> here.
>
> Either way, this is a job for host_ip() as found in pfctl or bgpd.
> Other daemons like iked still have host_v4() and host_v6(). Parsers use
> these functions to check for valid addresses, so I'd say sasyncd should
> be no exception and adopt the same approach.
>
> > @@ -325,7 +386,7 @@ yylex(void)
> > /* Numerical token? */
> > if (isdigit(*confptr)) {
> > for (p = confptr; *p; p++)
> > - if (*p == '.') /* IP address, or bad input */
> > + if (*p == '.' || *p == ':') /* IP address, or bad input
> > */
> This fixes the parser as in
>
> # echo peer 2001:db8::1 > conf
> # sasyncd -dnv -c conf
> config: syntax error
> # ./obj/sasyncd -dnv -c conf
> configuration OK
>
> But I have not actually tested this with a proper IPv6 setup since I do
> not use sasyncd; did you?
>
> > @@ -397,6 +458,9 @@ conf_parse_file(char *cfgfile)
> > if (*s == '#') {
> > while (*s != '\n' && s < buf + conflen)
> > s++;
> > + while (*s == '\n' && s < buf + conflen)
> > + s++;
> > + s--;
> OK kn for this fix alone.
>
I'll test an ipv6 seup and commit the lexer parts if those work.
Michal, can you work on the first set of comments? I think Klemens is
right.
-Otto
Re: [patch] Re: Possible sasyncd memory leak ?
On Tue, Mar 12, 2019 at 03:19:56PM +0100, Otto Moerbeek wrote:
> > I also fixed a case of parsing IPv6 addresses.
> >
> > Anyone willing to ok?
See comments inline.
> And now also with a lexer bug fixed. Earlier I thougt it was an order
> dependency in the clauses. But is was an order dependency in comment
> lines and empty lines.
> +check_peer_addr(const char *peer_addr)
> +{
> + int valid = 1;
> + short peer_family = AF_UNSPEC;
> + struct ifaddrs *ifap = NULL, *ifa;
> + struct syncpeer *peer;
> + struct sockaddr_in sa;
> + struct sockaddr_in6 sa6;
> +
> + if (inet_pton(AF_INET, peer_addr, &sa.sin_addr) == 1)
> + peer_family = AF_INET;
> +
> + if (peer_family == AF_UNSPEC && inet_pton(AF_INET6, peer_addr,
> + &sa6.sin6_addr) == 1)
> + peer_family = AF_INET6;
`peer_addr' must not be a CIDR network, so I'd go with the more AF
agnostic getaddrinfo(3) and check for res->ai_family in any case...
> + if (peer_family == AF_UNSPEC) {
> + log_msg(2, "config: skip unparseable peer %s", peer_addr);
> + valid = 0;
> + }
...but `peer_addr' may also be a hostname, so that is not handled by
your diff:
net.h: char*name; /* FQDN or an IP, from conf */
getaddrinfo(3) can resolve however, thus inet_pton(3) should not be used
here.
Either way, this is a job for host_ip() as found in pfctl or bgpd.
Other daemons like iked still have host_v4() and host_v6(). Parsers use
these functions to check for valid addresses, so I'd say sasyncd should
be no exception and adopt the same approach.
> @@ -325,7 +386,7 @@ yylex(void)
> /* Numerical token? */
> if (isdigit(*confptr)) {
> for (p = confptr; *p; p++)
> - if (*p == '.') /* IP address, or bad input */
> + if (*p == '.' || *p == ':') /* IP address, or bad input
> */
This fixes the parser as in
# echo peer 2001:db8::1 > conf
# sasyncd -dnv -c conf
config: syntax error
# ./obj/sasyncd -dnv -c conf
configuration OK
But I have not actually tested this with a proper IPv6 setup since I do
not use sasyncd; did you?
> @@ -397,6 +458,9 @@ conf_parse_file(char *cfgfile)
> if (*s == '#') {
> while (*s != '\n' && s < buf + conflen)
> s++;
> + while (*s == '\n' && s < buf + conflen)
> + s++;
> + s--;
OK kn for this fix alone.
Re: [patch] Re: Possible sasyncd memory leak ?
On Fri, Mar 15, 2019 at 12:46:32PM +0100, Michał Koc wrote:
> W dniu 12.03.2019 o 15:19, Otto Moerbeek pisze:
> > On Tue, Mar 12, 2019 at 02:02:15PM +0100, Otto Moerbeek wrote:
> >
> > > On Mon, Mar 11, 2019 at 05:11:56PM +0100, Otto Moerbeek wrote:
> > >
> > > > I was going to test your diff but it does not apply. Your mailer seems
> > > > to convert spaces and or tabs to funny char sequences. Please fix
> > > > (test by mailing to yourself and applying with patch(1)) and resend,
> > > >
> > > > -Otto
> > > >
> > > So I reworked the diff to apply and use the proper formatting
> > > (regulart sapces and tabs instead of UTF-8 non breaking spaces).
> > >
> > > I also fixed a case of parsing IPv6 addresses.
> > >
> > > Anyone willing to ok?
> > And now also with a lexer bug fixed. Earlier I thougt it was an order
> > dependency in the clauses. But is was an order dependency in comment
> > lines and empty lines.
> >
> > -Otto
> >
> >
> > Index: conf.y
> > ===
> > RCS file: /cvs/src/usr.sbin/sasyncd/conf.y,v
> > retrieving revision 1.19
> > diff -u -p -r1.19 conf.y
> > --- conf.y 9 Apr 2017 02:40:24 - 1.19
> > +++ conf.y 12 Mar 2019 14:16:23 -
> > @@ -30,8 +30,10 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -48,6 +50,7 @@ struct cfgstate cfgstate;
> > int conflen = 0;
> > char *confbuf, *confptr;
> > +int check_peer_addr(const char *);
> > int yyparse(void);
> > int yylex(void);
> > void yyerror(const char *);
> > @@ -172,17 +175,8 @@ setting: INTERFACE STRING
> > | PEER STRING
> > {
> > struct syncpeer *peer;
> > - int duplicate = 0;
> > - for (peer = LIST_FIRST(&cfgstate.peerlist); peer;
> > -peer = LIST_NEXT(peer, link))
> > - if (strcmp($2, peer->name) == 0) {
> > - duplicate++;
> > - break;
> > - }
> > - if (duplicate)
> > - free($2);
> > - else {
> > + if (check_peer_addr($2)) {
> > peer = calloc(1, sizeof *peer);
> > if (!peer) {
> > log_err("config: calloc(1, %lu) "
> > @@ -191,10 +185,11 @@ setting : INTERFACE STRING
> > YYERROR;
> > }
> > peer->name = $2;
> > - }
> > - LIST_INSERT_HEAD(&cfgstate.peerlist, peer, link);
> > - cfgstate.peercnt++;
> > - log_msg(2, "config: add peer %s", peer->name);
> > + LIST_INSERT_HEAD(&cfgstate.peerlist, peer,
> > link);
> > + cfgstate.peercnt++;
> > + log_msg(2, "config: add peer %s", peer->name);
> > + } else
> > + free($2);
> > }
> > | LISTEN ON STRING af port
> > {
> > @@ -284,6 +279,72 @@ match(char *token)
> > }
> > int
> > +check_peer_addr(const char *peer_addr)
> > +{
> > + int valid = 1;
> > + short peer_family = AF_UNSPEC;
> > + struct ifaddrs *ifap = NULL, *ifa;
> > + struct syncpeer *peer;
> > + struct sockaddr_in sa;
> > + struct sockaddr_in6 sa6;
> > +
> > + if (inet_pton(AF_INET, peer_addr, &sa.sin_addr) == 1)
> > + peer_family = AF_INET;
> > +
> > + if (peer_family == AF_UNSPEC && inet_pton(AF_INET6, peer_addr,
> > + &sa6.sin6_addr) == 1)
> > + peer_family = AF_INET6;
> > +
> > + if (peer_family == AF_UNSPEC) {
> > + log_msg(2, "config: skip unparseable peer %s", peer_addr);
> > + valid = 0;
> > + }
> > +
> > + if (valid && getifaddrs(&ifap) == 0) {
> > + for (ifa = ifap; ifa; ifa = ifa->ifa_next) {
> > + if (!ifa->ifa_addr || ifa->ifa_addr->sa_family !=
> > + peer_family)
> > + continue;
> > +
> > + switch (ifa->ifa_addr->sa_family) {
> > + case AF_INET:
> > + if (memcmp(&sa.sin_addr,
> > + &((struct sockaddr_in
> > *)ifa->ifa_addr)->sin_addr,
> > + sizeof(struct in_addr)) == 0)
> > + valid = 0;
> > + break;
> > + case AF_INET6:
> > + if (memcmp(&sa6.sin6_addr,
> > + &((struct sockaddr_in6
> > *)ifa->ifa_addr)->sin6_addr,
> > +
Re: [patch] Re: Possible sasyncd memory leak ?
W dniu 12.03.2019 o 15:19, Otto Moerbeek pisze:
On Tue, Mar 12, 2019 at 02:02:15PM +0100, Otto Moerbeek wrote:
On Mon, Mar 11, 2019 at 05:11:56PM +0100, Otto Moerbeek wrote:
I was going to test your diff but it does not apply. Your mailer seems
to convert spaces and or tabs to funny char sequences. Please fix
(test by mailing to yourself and applying with patch(1)) and resend,
-Otto
So I reworked the diff to apply and use the proper formatting
(regulart sapces and tabs instead of UTF-8 non breaking spaces).
I also fixed a case of parsing IPv6 addresses.
Anyone willing to ok?
And now also with a lexer bug fixed. Earlier I thougt it was an order
dependency in the clauses. But is was an order dependency in comment
lines and empty lines.
-Otto
Index: conf.y
===
RCS file: /cvs/src/usr.sbin/sasyncd/conf.y,v
retrieving revision 1.19
diff -u -p -r1.19 conf.y
--- conf.y 9 Apr 2017 02:40:24 - 1.19
+++ conf.y 12 Mar 2019 14:16:23 -
@@ -30,8 +30,10 @@
#include
#include
#include
+#include
#include
#include
+#include
#include
#include
#include
@@ -48,6 +50,7 @@ struct cfgstate cfgstate;
int conflen = 0;
char *confbuf, *confptr;
+int check_peer_addr(const char *);
int yyparse(void);
int yylex(void);
void yyerror(const char *);
@@ -172,17 +175,8 @@ setting: INTERFACE STRING
| PEER STRING
{
struct syncpeer *peer;
- int duplicate = 0;
- for (peer = LIST_FIRST(&cfgstate.peerlist); peer;
-peer = LIST_NEXT(peer, link))
- if (strcmp($2, peer->name) == 0) {
- duplicate++;
- break;
- }
- if (duplicate)
- free($2);
- else {
+ if (check_peer_addr($2)) {
peer = calloc(1, sizeof *peer);
if (!peer) {
log_err("config: calloc(1, %lu) "
@@ -191,10 +185,11 @@ setting : INTERFACE STRING
YYERROR;
}
peer->name = $2;
- }
- LIST_INSERT_HEAD(&cfgstate.peerlist, peer, link);
- cfgstate.peercnt++;
- log_msg(2, "config: add peer %s", peer->name);
+ LIST_INSERT_HEAD(&cfgstate.peerlist, peer,
link);
+ cfgstate.peercnt++;
+ log_msg(2, "config: add peer %s", peer->name);
+ } else
+ free($2);
}
| LISTEN ON STRING af port
{
@@ -284,6 +279,72 @@ match(char *token)
}
int
+check_peer_addr(const char *peer_addr)
+{
+ int valid = 1;
+ short peer_family = AF_UNSPEC;
+ struct ifaddrs *ifap = NULL, *ifa;
+ struct syncpeer *peer;
+ struct sockaddr_in sa;
+ struct sockaddr_in6 sa6;
+
+ if (inet_pton(AF_INET, peer_addr, &sa.sin_addr) == 1)
+ peer_family = AF_INET;
+
+ if (peer_family == AF_UNSPEC && inet_pton(AF_INET6, peer_addr,
+ &sa6.sin6_addr) == 1)
+ peer_family = AF_INET6;
+
+ if (peer_family == AF_UNSPEC) {
+ log_msg(2, "config: skip unparseable peer %s", peer_addr);
+ valid = 0;
+ }
+
+ if (valid && getifaddrs(&ifap) == 0) {
+ for (ifa = ifap; ifa; ifa = ifa->ifa_next) {
+ if (!ifa->ifa_addr || ifa->ifa_addr->sa_family !=
+ peer_family)
+ continue;
+
+ switch (ifa->ifa_addr->sa_family) {
+ case AF_INET:
+ if (memcmp(&sa.sin_addr,
+ &((struct sockaddr_in
*)ifa->ifa_addr)->sin_addr,
+ sizeof(struct in_addr)) == 0)
+ valid = 0;
+ break;
+ case AF_INET6:
+ if (memcmp(&sa6.sin6_addr,
+ &((struct sockaddr_in6
*)ifa->ifa_addr)->sin6_addr,
+ sizeof(struct in6_addr)) == 0)
+ valid = 0;
+ break;
+ }
+
+ if (!valid) {
+ log_msg(2, "config: skip local peer %s",
+ peer_addr);
+ break;
+
Re: [patch] Re: Possible sasyncd memory leak ?
On Tue, Mar 12, 2019 at 02:02:15PM +0100, Otto Moerbeek wrote:
> On Mon, Mar 11, 2019 at 05:11:56PM +0100, Otto Moerbeek wrote:
>
> >
> > I was going to test your diff but it does not apply. Your mailer seems
> > to convert spaces and or tabs to funny char sequences. Please fix
> > (test by mailing to yourself and applying with patch(1)) and resend,
> >
> > -Otto
> >
>
> So I reworked the diff to apply and use the proper formatting
> (regulart sapces and tabs instead of UTF-8 non breaking spaces).
>
> I also fixed a case of parsing IPv6 addresses.
>
> Anyone willing to ok?
And now also with a lexer bug fixed. Earlier I thougt it was an order
dependency in the clauses. But is was an order dependency in comment
lines and empty lines.
-Otto
Index: conf.y
===
RCS file: /cvs/src/usr.sbin/sasyncd/conf.y,v
retrieving revision 1.19
diff -u -p -r1.19 conf.y
--- conf.y 9 Apr 2017 02:40:24 - 1.19
+++ conf.y 12 Mar 2019 14:16:23 -
@@ -30,8 +30,10 @@
#include
#include
#include
+#include
#include
#include
+#include
#include
#include
#include
@@ -48,6 +50,7 @@ struct cfgstate cfgstate;
intconflen = 0;
char *confbuf, *confptr;
+int check_peer_addr(const char *);
intyyparse(void);
intyylex(void);
void yyerror(const char *);
@@ -172,17 +175,8 @@ setting: INTERFACE STRING
| PEER STRING
{
struct syncpeer *peer;
- int duplicate = 0;
- for (peer = LIST_FIRST(&cfgstate.peerlist); peer;
-peer = LIST_NEXT(peer, link))
- if (strcmp($2, peer->name) == 0) {
- duplicate++;
- break;
- }
- if (duplicate)
- free($2);
- else {
+ if (check_peer_addr($2)) {
peer = calloc(1, sizeof *peer);
if (!peer) {
log_err("config: calloc(1, %lu) "
@@ -191,10 +185,11 @@ setting : INTERFACE STRING
YYERROR;
}
peer->name = $2;
- }
- LIST_INSERT_HEAD(&cfgstate.peerlist, peer, link);
- cfgstate.peercnt++;
- log_msg(2, "config: add peer %s", peer->name);
+ LIST_INSERT_HEAD(&cfgstate.peerlist, peer,
link);
+ cfgstate.peercnt++;
+ log_msg(2, "config: add peer %s", peer->name);
+ } else
+ free($2);
}
| LISTEN ON STRING af port
{
@@ -284,6 +279,72 @@ match(char *token)
}
int
+check_peer_addr(const char *peer_addr)
+{
+ int valid = 1;
+ short peer_family = AF_UNSPEC;
+ struct ifaddrs *ifap = NULL, *ifa;
+ struct syncpeer *peer;
+ struct sockaddr_in sa;
+ struct sockaddr_in6 sa6;
+
+ if (inet_pton(AF_INET, peer_addr, &sa.sin_addr) == 1)
+ peer_family = AF_INET;
+
+ if (peer_family == AF_UNSPEC && inet_pton(AF_INET6, peer_addr,
+ &sa6.sin6_addr) == 1)
+ peer_family = AF_INET6;
+
+ if (peer_family == AF_UNSPEC) {
+ log_msg(2, "config: skip unparseable peer %s", peer_addr);
+ valid = 0;
+ }
+
+ if (valid && getifaddrs(&ifap) == 0) {
+ for (ifa = ifap; ifa; ifa = ifa->ifa_next) {
+ if (!ifa->ifa_addr || ifa->ifa_addr->sa_family !=
+ peer_family)
+ continue;
+
+ switch (ifa->ifa_addr->sa_family) {
+ case AF_INET:
+ if (memcmp(&sa.sin_addr,
+ &((struct sockaddr_in
*)ifa->ifa_addr)->sin_addr,
+ sizeof(struct in_addr)) == 0)
+ valid = 0;
+ break;
+ case AF_INET6:
+ if (memcmp(&sa6.sin6_addr,
+ &((struct sockaddr_in6
*)ifa->ifa_addr)->sin6_addr,
+ sizeof(struct in6_addr)) == 0)
+ valid = 0;
+ break;
+ }
+
+ if (!valid) {
+ log_msg(2, "config: skip local peer %s",
+ peer_addr);
+ break;
+
Re: [patch] Re: Possible sasyncd memory leak ?
On Mon, Mar 11, 2019 at 05:11:56PM +0100, Otto Moerbeek wrote:
>
> I was going to test your diff but it does not apply. Your mailer seems
> to convert spaces and or tabs to funny char sequences. Please fix
> (test by mailing to yourself and applying with patch(1)) and resend,
>
> -Otto
>
So I reworked the diff to apply and use the proper formatting
(regulart sapces and tabs instead of UTF-8 non breaking spaces).
I also fixed a case of parsing IPv6 addresses.
Anyone willing to ok?
-Otto
Index: conf.y
===
RCS file: /cvs/src/usr.sbin/sasyncd/conf.y,v
retrieving revision 1.19
diff -u -p -r1.19 conf.y
--- conf.y 9 Apr 2017 02:40:24 - 1.19
+++ conf.y 12 Mar 2019 12:59:18 -
@@ -30,8 +30,10 @@
#include
#include
#include
+#include
#include
#include
+#include
#include
#include
#include
@@ -48,6 +50,7 @@ struct cfgstate cfgstate;
intconflen = 0;
char *confbuf, *confptr;
+int check_peer_addr(const char *);
intyyparse(void);
intyylex(void);
void yyerror(const char *);
@@ -172,17 +175,8 @@ setting: INTERFACE STRING
| PEER STRING
{
struct syncpeer *peer;
- int duplicate = 0;
- for (peer = LIST_FIRST(&cfgstate.peerlist); peer;
-peer = LIST_NEXT(peer, link))
- if (strcmp($2, peer->name) == 0) {
- duplicate++;
- break;
- }
- if (duplicate)
- free($2);
- else {
+ if (check_peer_addr($2)) {
peer = calloc(1, sizeof *peer);
if (!peer) {
log_err("config: calloc(1, %lu) "
@@ -191,10 +185,11 @@ setting : INTERFACE STRING
YYERROR;
}
peer->name = $2;
- }
- LIST_INSERT_HEAD(&cfgstate.peerlist, peer, link);
- cfgstate.peercnt++;
- log_msg(2, "config: add peer %s", peer->name);
+ LIST_INSERT_HEAD(&cfgstate.peerlist, peer,
link);
+ cfgstate.peercnt++;
+ log_msg(2, "config: add peer %s", peer->name);
+ } else
+ free($2);
}
| LISTEN ON STRING af port
{
@@ -284,6 +279,72 @@ match(char *token)
}
int
+check_peer_addr(const char *peer_addr)
+{
+ int valid = 1;
+ short peer_family = AF_UNSPEC;
+ struct ifaddrs *ifap = 0, *ifa;
+ struct syncpeer *peer;
+ struct sockaddr_in sa;
+ struct sockaddr_in6 sa6;
+
+ if (inet_pton(AF_INET, peer_addr, &sa.sin_addr) == 1)
+ peer_family = AF_INET;
+
+ if (peer_family == AF_UNSPEC && inet_pton(AF_INET6, peer_addr,
+ &sa6.sin6_addr) == 1)
+ peer_family = AF_INET6;
+
+ if (peer_family == AF_UNSPEC) {
+ log_msg(2, "config: skip unparseable peer %s", peer_addr);
+ valid = 0;
+ }
+
+ if (valid && getifaddrs(&ifap) == 0) {
+ for (ifa = ifap; ifa; ifa = ifa->ifa_next) {
+ if (!ifa->ifa_addr || ifa->ifa_addr->sa_family !=
+ peer_family)
+ continue;
+
+ switch (ifa->ifa_addr->sa_family) {
+ case AF_INET:
+ if (memcmp(&sa.sin_addr,
+ &((struct sockaddr_in
*)ifa->ifa_addr)->sin_addr,
+ sizeof(struct in_addr)) == 0)
+ valid = 0;
+ break;
+ case AF_INET6:
+ if (memcmp(&sa6.sin6_addr,
+ &((struct sockaddr_in6
*)ifa->ifa_addr)->sin6_addr,
+ sizeof(struct in6_addr)) == 0)
+ valid = 0;
+ break;
+ }
+
+ if (!valid) {
+ log_msg(2, "config: skip local peer %s",
+ peer_addr);
+ break;
+ }
+ }
+ freeifaddrs(ifap);
+ }
+
+ if (valid) {
+ for (peer = LIST_FIRST(&cfgstate.peerlist); peer;
+ peer = LIST_NEXT(peer, link)) {
+ if (strcmp(peer_a
Re: [patch] Re: Possible sasyncd memory leak ?
I was going to test your diff but it does not apply. Your mailer seems to convert spaces and or tabs to funny char sequences. Please fix (test by mailing to yourself and applying with patch(1)) and resend, -Otto
Re: [patch] Re: Possible sasyncd memory leak ?
W dniu 11.03.2019 o 09:29, Otto Moerbeek pisze: On Sun, Mar 10, 2019 at 02:41:35PM +0100, Michał Koc wrote: W dniu 10.03.2019 o 08:10, Otto Moerbeek pisze: On Sat, Mar 09, 2019 at 11:19:11PM +0100, Michał Koc wrote: W dniu 09.03.2019 o 12:43, Otto Moerbeek pisze: On Sat, Mar 09, 2019 at 12:10:34PM +0100, Michał Koc wrote: W dniu 09.03.2019 o 08:15, Otto Moerbeek pisze: On Fri, Mar 08, 2019 at 12:03:25PM +0100, Michał Koc wrote: Hi all, We have a triple redundant vpn gateway setup with sasyncd running and tons of tunnels, about 1000 flows. Looking at the graph of memory usage, you can clearly see that something is sucking up the memory. The graph can be viewed here: https://pasteboard.co/I4sjzQ8.jpg Looking at the ps, sasyncd shows huge memory consumption: USER PID %CPU %MEM VSZ RSS TT STAT STARTED TIME COMMAND _isakmpd 33560 0.0 17.0 699264 708508 ?? S 26Feb19 6:58.81 /usr/sbin/sasyncd It only happens on the master node. Slaves do not show such a behavior. There is nothing about sasyncd in the logs. After sasyncd restart memory consumption is minimal, but tends to grow. Is it normal ? or am I missing something ? Best regards M.K. This is not normal. You could try to run with -vv to see if some error path is taken that triggers a leak. -Otto Should I look for something specific ? The log grows pretty fast and it looks like it could contain some security data which I wouldn't like to post online. The statistics of the log(about 2 hours) looks like this: carp_init: 1 config: 7 monitor_get_pfkey_snap: 4 monitor_loop: 1 net: 1 net_connect: 3 net_ctl: 4 net_disconnect_peer: 3 net_handle_messages: 2 net_queue: 91780 net_read: 10 net_send_messages: 39192 pfkey_send_flush: 4 pfkey_snapshot: 6832 timer_add: 19 timer_run: 18 Best regards M.K. Just the counts does not reveal anything. I did a quick audit of the memory allocation logic of sasyncd and did not spot an error. If you do not want to post the logs, you'll neeed to analyze them yourself. This requires matching the log lines to the code and tracking where stuff gets allocated and deallocated. Some digging could reveal the error. I used to run sasyncd, but I do no longer. Settig up a test env is quite some work I do not have time for. -Otto First of all, thank You for your time. I know it's one of the most valuable resource. We have done some analysis and we have found the problem. The problem is that the very master machine exists as a peer in it's config. The purpose of this is to make all of the config files to be as similar as possible for all of the members of the cluster. Removing it from peers fixes the problem. So there are three main roads we can follow: 1. Fix sasyncd to recognize self and handle it properly (a result) 2. It should be mentioned in manual, not to set self as a peer (an excuse) 3. We can change our internal config handling (no one else benefits) What would You recommend as a next step ? we will do much to follow road 1, but we might need help, eg. code review and some guidance to meet OpenBSD needs Furthermore if we follow road 1, is there any chance to get the reviewed, correct, accepted fix into the tree ? Best regards M.K. Good you found the problem. As for number 1, in general it is hard to determine if an IP is your own (or redirects to yourself), especially since network interfaces can be added and changed dynamically. But a starup check against getifaddrs(3) would catch the most obvious I'm my own peer cases. A dynamic form of loop detection would be nice, it would require adding a hostid or equivalent. That is not normally set these days and also would change the wire format of the messages. But even if you start work on code, a manual page change warning against this would be a good thing. If you post diffs to tech@ with a proper explanation they will be picked up and reviewed, either by me or somee other developer. BTW, I noticed there are order dependencies in sasyncd.cnf that are not mentioned in the man page. Also the example in src/etc/examples/sasyncd.conf does not work if you just uncomment the lines because of those order dependencies. So there's more room for improvement in the man page and example config. -Otto Correct me please if I am wrong, but in my opinion vpn gateways are usually static setups, because of design, security, network, documentation, politics and other reasons So startup check should at least give a clue when something goes wrong. Of course setting up some kind of host id would be best solution, we will get back to it in less busy time. The patch below makes the following changes: 1. adds startup check for peers being listed in local addresses 2. adds proper log messages in cases of peer being local address or duplicate 3. fixes duplicate handling, which was incorrect 4. ch
Re: [patch] Re: Possible sasyncd memory leak ?
On Sun, Mar 10, 2019 at 02:41:35PM +0100, Michał Koc wrote: > W dniu 10.03.2019 o 08:10, Otto Moerbeek pisze: > > On Sat, Mar 09, 2019 at 11:19:11PM +0100, Michał Koc wrote: > > > > > W dniu 09.03.2019 o 12:43, Otto Moerbeek pisze: > > > > On Sat, Mar 09, 2019 at 12:10:34PM +0100, Michał Koc wrote: > > > > > > > > > W dniu 09.03.2019 o 08:15, Otto Moerbeek pisze: > > > > > > On Fri, Mar 08, 2019 at 12:03:25PM +0100, Michał Koc wrote: > > > > > > > > > > > > > Hi all, > > > > > > > > > > > > > > We have a triple redundant vpn gateway setup with sasyncd running > > > > > > > and tons > > > > > > > of tunnels, about 1000 flows. > > > > > > > > > > > > > > Looking at the graph of memory usage, you can clearly see that > > > > > > > something is > > > > > > > sucking up the memory. > > > > > > > > > > > > > > The graph can be viewed here: https://pasteboard.co/I4sjzQ8.jpg > > > > > > > > > > > > > > Looking at the ps, sasyncd shows huge memory consumption: > > > > > > > > > > > > > > USER PID %CPU %MEM VSZ RSS TT > > > > > > > STAT > > > > > > > STARTED TIME COMMAND > > > > > > > _isakmpd 33560 0.0 17.0 699264 708508 ?? S > > > > > > > 26Feb19 6:58.81 /usr/sbin/sasyncd > > > > > > > > > > > > > > It only happens on the master node. Slaves do not show such a > > > > > > > behavior. > > > > > > > > > > > > > > There is nothing about sasyncd in the logs. > > > > > > > > > > > > > > After sasyncd restart memory consumption is minimal, but tends to > > > > > > > grow. > > > > > > > > > > > > > > Is it normal ? or am I missing something ? > > > > > > > > > > > > > > Best regards > > > > > > > M.K. > > > > > > > > > > > > > This is not normal. You could try to run with -vv to see if some > > > > > > error > > > > > > path is taken that triggers a leak. > > > > > > > > > > > > -Otto > > > > > > > > > > > Should I look for something specific ? > > > > > > > > > > The log grows pretty fast and it looks like it could contain some > > > > > security > > > > > data which I wouldn't like to post online. > > > > > > > > > > The statistics of the log(about 2 hours) looks like this: > > > > > carp_init: 1 > > > > > config: 7 > > > > > monitor_get_pfkey_snap: 4 > > > > > monitor_loop: 1 > > > > > net: 1 > > > > > net_connect: 3 > > > > > net_ctl: 4 > > > > > net_disconnect_peer: 3 > > > > > net_handle_messages: 2 > > > > > net_queue: 91780 > > > > > net_read: 10 > > > > > net_send_messages: 39192 > > > > > pfkey_send_flush: 4 > > > > > pfkey_snapshot: 6832 > > > > > timer_add: 19 > > > > > timer_run: 18 > > > > > > > > > > Best regards > > > > > M.K. > > > > > > > > > Just the counts does not reveal anything. I did a quick audit of the > > > > memory allocation logic of sasyncd and did not spot an error. If you > > > > do not want to post the logs, you'll neeed to analyze them yourself. > > > > This requires matching the log lines to the code and tracking where > > > > stuff gets allocated and deallocated. Some digging could reveal the > > > > error. > > > > > > > > I used to run sasyncd, but I do no longer. Settig up a test env is > > > > quite some work I do not have time for. > > > > > > > > -Otto > > > > > > > First of all, thank You for your time. I know it's one of the most > > > valuable > > > resource. > > > > > > We have done some analysis and we have found the problem. > > > > > > The problem is that the very master machine exists as a peer in it's > > > config. > > > The purpose of this is to make all of the config files to be as similar as > > > possible for all of the members of the cluster. > > > > > > Removing it from peers fixes the problem. > > > > > > So there are three main roads we can follow: > > > 1. Fix sasyncd to recognize self and handle it properly (a result) > > > 2. It should be mentioned in manual, not to set self as a peer (an excuse) > > > 3. We can change our internal config handling (no one else benefits) > > > > > > What would You recommend as a next step ? we will do much to follow road > > > 1, > > > but we might need help, eg. code review and some guidance to meet OpenBSD > > > needs > > > > > > Furthermore if we follow road 1, is there any chance to get the reviewed, > > > correct, accepted fix into the tree ? > > > > > > Best regards > > > M.K. > > > > > Good you found the problem. > > > > As for number 1, in general it is hard to determine if an IP is your > > own (or redirects to yourself), especially since network interfaces > > can be added and changed dynamically. But a starup check against > > getifaddrs(3) would catch the most obvious I'm my own peer cases. > > > > A dynamic form of loop detection would be nice, it would require > > adding a hostid or equivalent. That is not normally set these days and > > also would change the wire format of the messages. > > > > But even if yo
[patch] Re: Possible sasyncd memory leak ?
W dniu 10.03.2019 o 08:10, Otto Moerbeek pisze: On Sat, Mar 09, 2019 at 11:19:11PM +0100, Michał Koc wrote: W dniu 09.03.2019 o 12:43, Otto Moerbeek pisze: On Sat, Mar 09, 2019 at 12:10:34PM +0100, Michał Koc wrote: W dniu 09.03.2019 o 08:15, Otto Moerbeek pisze: On Fri, Mar 08, 2019 at 12:03:25PM +0100, Michał Koc wrote: Hi all, We have a triple redundant vpn gateway setup with sasyncd running and tons of tunnels, about 1000 flows. Looking at the graph of memory usage, you can clearly see that something is sucking up the memory. The graph can be viewed here: https://pasteboard.co/I4sjzQ8.jpg Looking at the ps, sasyncd shows huge memory consumption: USER PID %CPU %MEM VSZ RSS TT STAT STARTED TIME COMMAND _isakmpd 33560 0.0 17.0 699264 708508 ?? S 26Feb19 6:58.81 /usr/sbin/sasyncd It only happens on the master node. Slaves do not show such a behavior. There is nothing about sasyncd in the logs. After sasyncd restart memory consumption is minimal, but tends to grow. Is it normal ? or am I missing something ? Best regards M.K. This is not normal. You could try to run with -vv to see if some error path is taken that triggers a leak. -Otto Should I look for something specific ? The log grows pretty fast and it looks like it could contain some security data which I wouldn't like to post online. The statistics of the log(about 2 hours) looks like this: carp_init: 1 config: 7 monitor_get_pfkey_snap: 4 monitor_loop: 1 net: 1 net_connect: 3 net_ctl: 4 net_disconnect_peer: 3 net_handle_messages: 2 net_queue: 91780 net_read: 10 net_send_messages: 39192 pfkey_send_flush: 4 pfkey_snapshot: 6832 timer_add: 19 timer_run: 18 Best regards M.K. Just the counts does not reveal anything. I did a quick audit of the memory allocation logic of sasyncd and did not spot an error. If you do not want to post the logs, you'll neeed to analyze them yourself. This requires matching the log lines to the code and tracking where stuff gets allocated and deallocated. Some digging could reveal the error. I used to run sasyncd, but I do no longer. Settig up a test env is quite some work I do not have time for. -Otto First of all, thank You for your time. I know it's one of the most valuable resource. We have done some analysis and we have found the problem. The problem is that the very master machine exists as a peer in it's config. The purpose of this is to make all of the config files to be as similar as possible for all of the members of the cluster. Removing it from peers fixes the problem. So there are three main roads we can follow: 1. Fix sasyncd to recognize self and handle it properly (a result) 2. It should be mentioned in manual, not to set self as a peer (an excuse) 3. We can change our internal config handling (no one else benefits) What would You recommend as a next step ? we will do much to follow road 1, but we might need help, eg. code review and some guidance to meet OpenBSD needs Furthermore if we follow road 1, is there any chance to get the reviewed, correct, accepted fix into the tree ? Best regards M.K. Good you found the problem. As for number 1, in general it is hard to determine if an IP is your own (or redirects to yourself), especially since network interfaces can be added and changed dynamically. But a starup check against getifaddrs(3) would catch the most obvious I'm my own peer cases. A dynamic form of loop detection would be nice, it would require adding a hostid or equivalent. That is not normally set these days and also would change the wire format of the messages. But even if you start work on code, a manual page change warning against this would be a good thing. If you post diffs to tech@ with a proper explanation they will be picked up and reviewed, either by me or somee other developer. BTW, I noticed there are order dependencies in sasyncd.cnf that are not mentioned in the man page. Also the example in src/etc/examples/sasyncd.conf does not work if you just uncomment the lines because of those order dependencies. So there's more room for improvement in the man page and example config. -Otto Correct me please if I am wrong, but in my opinion vpn gateways are usually static setups, because of design, security, network, documentation, politics and other reasons So startup check should at least give a clue when something goes wrong. Of course setting up some kind of host id would be best solution, we will get back to it in less busy time. The patch below makes the following changes: 1. adds startup check for peers being listed in local addresses 2. adds proper log messages in cases of peer being local address or duplicate 3. fixes duplicate handling, which was incorrect 4. changes name of "duplicate" variable to "valid" to allow it to used in more cases than just duplicate B
