[patch] Re: Possible sasyncd memory leak ?

classic Classic list List threaded Threaded
23 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[patch] Re: Possible sasyncd memory leak ?

Michał Koc
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

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 -0000 1.19
+++ conf.y 10 Mar 2019 13:39:04 -0000
@@ -30,8 +30,10 @@
  #include <sys/types.h>
  #include <sys/stat.h>
  #include <sys/socket.h>
+#include <arpa/inet.h>
  #include <ctype.h>
  #include <fcntl.h>
+#include <ifaddrs.h>
  #include <stdio.h>
  #include <stdlib.h>
  #include <string.h>
@@ -172,17 +174,50 @@ setting : INTERFACE STRING
  | PEER STRING
  {
  struct syncpeer *peer;
- int duplicate = 0;
+ int valid = 1;
+ struct ifaddrs *ifap = 0, *ifa;
+ char *ifaddrname = 0;
+
+ if (getifaddrs(&ifap) == 0) {
+ for (ifa = ifap; ifa && valid; ifa = ifa->ifa_next) {
+ if (!ifa->ifa_addr)
+ continue;
+
+ switch (ifa->ifa_addr->sa_family) {
+ case AF_INET:
+ ifaddrname = malloc(INET_ADDRSTRLEN);
+ if (ifaddrname)
+ inet_ntop(AF_INET, &((struct sockaddr_in *)ifa->ifa_addr)->sin_addr, ifaddrname, INET_ADDRSTRLEN);
+ break;
+ case AF_INET6:
+ ifaddrname = malloc(INET6_ADDRSTRLEN);
+ if (ifaddrname)
+ inet_ntop(AF_INET6, &((struct sockaddr_in6 *)ifa->ifa_addr)->sin6_addr, ifaddrname, INET6_ADDRSTRLEN);
+ break;
+ }
+
+ if(ifaddrname) {
+ if (strcmp($2, ifaddrname) == 0) {
+ log_msg(2, "config: local peer %s", $2);
+ valid = 0;
+ }
+ free(ifaddrname);
+ ifaddrname = 0;
+ }
+ }
+ freeifaddrs(ifap);
+ }
 
- for (peer = LIST_FIRST(&cfgstate.peerlist); peer;
-     peer = LIST_NEXT(peer, link))
- if (strcmp($2, peer->name) == 0) {
- duplicate++;
+ if (valid)
+    for (peer = LIST_FIRST(&cfgstate.peerlist); peer;
+ peer = LIST_NEXT(peer, link))
+    if (strcmp($2, peer->name) == 0) {
+ log_msg(2, "config: duplicate peer %s", $2);
+ valid = 0;
  break;
- }
- if (duplicate)
- free($2);
- else {
+    }
+
+ if (valid) {
  peer = calloc(1, sizeof *peer);
  if (!peer) {
  log_err("config: calloc(1, %lu) "
@@ -191,10 +226,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
  {

Reply | Threaded
Open this post in threaded view
|

Re: [patch] Re: Possible sasyncd memory leak ?

Otto Moerbeek
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. changes name of "duplicate" variable to "valid" to allow it to used in
> more cases than just duplicate
>
> Best regards
> M.K.

Thanks for working on this,

Some comments:

1. I do not like lots of code in yacc rules. Better make the validation
a separate function.

2. You are comparing string representations. One is from the config
file and one is produced by inet_ntop(), that one is in canonical
form, but the first does not have to be. So you might miss equivalent
addresses.  Probably better convert the ones form the config file with
inet_pton() compare those to the addresses returned by getifaddrs()
using memcmp().

3. Why use malloc for fixed size? A char buf[INET6_ADDRSTRLEN] will do,
(but see 2).

4. I would make it clear in the log message you're skipping the "self" peer.

        -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 -0000 1.19
> +++ conf.y 10 Mar 2019 13:39:04 -0000
> @@ -30,8 +30,10 @@
>  #include <sys/types.h>
>  #include <sys/stat.h>
>  #include <sys/socket.h>
> +#include <arpa/inet.h>
>  #include <ctype.h>
>  #include <fcntl.h>
> +#include <ifaddrs.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> @@ -172,17 +174,50 @@ setting : INTERFACE STRING
>   | PEER STRING
>   {
>   struct syncpeer *peer;
> - int duplicate = 0;
> + int valid = 1;
> + struct ifaddrs *ifap = 0, *ifa;
> + char *ifaddrname = 0;
> +
> + if (getifaddrs(&ifap) == 0) {
> + for (ifa = ifap; ifa && valid; ifa = ifa->ifa_next) {
> + if (!ifa->ifa_addr)
> + continue;
> +
> + switch (ifa->ifa_addr->sa_family) {
> + case AF_INET:
> + ifaddrname = malloc(INET_ADDRSTRLEN);
> + if (ifaddrname)
> + inet_ntop(AF_INET, &((struct sockaddr_in *)ifa->ifa_addr)->sin_addr, ifaddrname, INET_ADDRSTRLEN);
> + break;
> + case AF_INET6:
> + ifaddrname = malloc(INET6_ADDRSTRLEN);
> + if (ifaddrname)
> + inet_ntop(AF_INET6, &((struct sockaddr_in6 *)ifa->ifa_addr)->sin6_addr, ifaddrname, INET6_ADDRSTRLEN);
> + break;
> + }
> +
> + if(ifaddrname) {
> + if (strcmp($2, ifaddrname) == 0) {
> + log_msg(2, "config: local peer %s", $2);
> + valid = 0;
> + }
> + free(ifaddrname);
> + ifaddrname = 0;
> + }
> + }
> + freeifaddrs(ifap);
> + }
> - for (peer = LIST_FIRST(&cfgstate.peerlist); peer;
> -     peer = LIST_NEXT(peer, link))
> - if (strcmp($2, peer->name) == 0) {
> - duplicate++;
> + if (valid)
> +    for (peer = LIST_FIRST(&cfgstate.peerlist); peer;
> + peer = LIST_NEXT(peer, link))
> +    if (strcmp($2, peer->name) == 0) {
> + log_msg(2, "config: duplicate peer %s", $2);
> + valid = 0;
>   break;
> - }
> - if (duplicate)
> - free($2);
> - else {
> +    }
> +
> + if (valid) {
>   peer = calloc(1, sizeof *peer);
>   if (!peer) {
>   log_err("config: calloc(1, %lu) "
> @@ -191,10 +226,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
>   {
>

Reply | Threaded
Open this post in threaded view
|

Re: [patch] Re: Possible sasyncd memory leak ?

Michał Koc
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. changes name of "duplicate" variable to "valid" to allow it to used in
>> more cases than just duplicate
>>
>> Best regards
>> M.K.
> Thanks for working on this,
>
> Some comments:
>
> 1. I do not like lots of code in yacc rules. Better make the validation
> a separate function.
>
> 2. You are comparing string representations. One is from the config
> file and one is produced by inet_ntop(), that one is in canonical
> form, but the first does not have to be. So you might miss equivalent
> addresses.  Probably better convert the ones form the config file with
> inet_pton() compare those to the addresses returned by getifaddrs()
> using memcmp().
>
> 3. Why use malloc for fixed size? A char buf[INET6_ADDRSTRLEN] will do,
> (but see 2).
>
> 4. I would make it clear in the log message you're skipping the "self" peer.
>
> -Otto

No problem. Thank You too :)

Ad. 1. Checking peer address moved to separate function. Three main
parts there:
     1. Prepare data
     2. Check for local
     3. Check for duplicate

Ad. 2. Done
Ad. 3. No longer relevant
Ad. 4. Done for all messages

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 -0000    1.19
+++ conf.y    11 Mar 2019 11:04:56 -0000
@@ -30,8 +30,10 @@
  #include <sys/types.h>
  #include <sys/stat.h>
  #include <sys/socket.h>
+#include <arpa/inet.h>
  #include <ctype.h>
  #include <fcntl.h>
+#include <ifaddrs.h>
  #include <stdio.h>
  #include <stdlib.h>
  #include <string.h>
@@ -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
          {
@@ -281,6 +276,64 @@ match(char *token)
          sizeof keywords[0], match_cmp);

      return k ? k->value : STRING;
+}
+
+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_addr, peer->name) == 0) {
+            log_msg(2, "config: skip duplicate peer %s", peer_addr);
+            valid = 0;
+            break;
+            }
+
+    return valid;
  }

  int

Reply | Threaded
Open this post in threaded view
|

Re: [patch] Re: Possible sasyncd memory leak ?

Otto Moerbeek

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

Reply | Threaded
Open this post in threaded view
|

Re: [patch] Re: Possible sasyncd memory leak ?

Otto Moerbeek
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 -0000 1.19
+++ conf.y 12 Mar 2019 12:59:18 -0000
@@ -30,8 +30,10 @@
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <sys/socket.h>
+#include <arpa/inet.h>
 #include <ctype.h>
 #include <fcntl.h>
+#include <ifaddrs.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -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 = 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_addr, peer->name) == 0) {
+ log_msg(2, "config: skip duplicate peer %s",
+    peer_addr);
+ valid = 0;
+ }
+ }
+ }
+
+ return valid;
+}
+
+int
 yylex(void)
 {
  char *p;
@@ -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 */
  goto is_string;
  v = (int)strtol(confptr, (char **)NULL, 10);
  yylval.val = v;

Reply | Threaded
Open this post in threaded view
|

Re: [patch] Re: Possible sasyncd memory leak ?

Otto Moerbeek
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 -0000 1.19
+++ conf.y 12 Mar 2019 14:16:23 -0000
@@ -30,8 +30,10 @@
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <sys/socket.h>
+#include <arpa/inet.h>
 #include <ctype.h>
 #include <fcntl.h>
+#include <ifaddrs.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -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;
+ }
+ }
+ freeifaddrs(ifap);
+ }
+
+ if (valid) {
+ for (peer = LIST_FIRST(&cfgstate.peerlist); peer;
+ peer = LIST_NEXT(peer, link)) {
+ if (strcmp(peer_addr, peer->name) == 0) {
+ log_msg(2, "config: skip duplicate peer %s",
+    peer_addr);
+ valid = 0;
+ }
+ }
+ }
+
+ return valid;
+}
+
+int
 yylex(void)
 {
  char *p;
@@ -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 */
  goto is_string;
  v = (int)strtol(confptr, (char **)NULL, 10);
  yylval.val = v;
@@ -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--;
  continue;
  }
  if (d == buf && isspace(*s))

Reply | Threaded
Open this post in threaded view
|

Re: [patch] Re: Possible sasyncd memory leak ?

Michał Koc
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 -0000 1.19
> +++ conf.y 12 Mar 2019 14:16:23 -0000
> @@ -30,8 +30,10 @@
>   #include <sys/types.h>
>   #include <sys/stat.h>
>   #include <sys/socket.h>
> +#include <arpa/inet.h>
>   #include <ctype.h>
>   #include <fcntl.h>
> +#include <ifaddrs.h>
>   #include <stdio.h>
>   #include <stdlib.h>
>   #include <string.h>
> @@ -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;
> + }
> + }
> + freeifaddrs(ifap);
> + }
> +
> + if (valid) {
> + for (peer = LIST_FIRST(&cfgstate.peerlist); peer;
> + peer = LIST_NEXT(peer, link)) {
> + if (strcmp(peer_addr, peer->name) == 0) {
> + log_msg(2, "config: skip duplicate peer %s",
> +    peer_addr);
> + valid = 0;
> + }
> + }
> + }
> +
> + return valid;
> +}
> +
> +int
>   yylex(void)
>   {
>   char *p;
> @@ -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 */
>   goto is_string;
>   v = (int)strtol(confptr, (char **)NULL, 10);
>   yylval.val = v;
> @@ -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--;
>   continue;
>   }
>   if (d == buf && isspace(*s))
>
>
Hi all,

What are the next steps to push it forward ?

Best regards
M.K.

Reply | Threaded
Open this post in threaded view
|

Re: [patch] Re: Possible sasyncd memory leak ?

Otto Moerbeek
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 -0000 1.19
> > +++ conf.y 12 Mar 2019 14:16:23 -0000
> > @@ -30,8 +30,10 @@
> >   #include <sys/types.h>
> >   #include <sys/stat.h>
> >   #include <sys/socket.h>
> > +#include <arpa/inet.h>
> >   #include <ctype.h>
> >   #include <fcntl.h>
> > +#include <ifaddrs.h>
> >   #include <stdio.h>
> >   #include <stdlib.h>
> >   #include <string.h>
> > @@ -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;
> > + }
> > + }
> > + freeifaddrs(ifap);
> > + }
> > +
> > + if (valid) {
> > + for (peer = LIST_FIRST(&cfgstate.peerlist); peer;
> > + peer = LIST_NEXT(peer, link)) {
> > + if (strcmp(peer_addr, peer->name) == 0) {
> > + log_msg(2, "config: skip duplicate peer %s",
> > +    peer_addr);
> > + valid = 0;
> > + }
> > + }
> > + }
> > +
> > + return valid;
> > +}
> > +
> > +int
> >   yylex(void)
> >   {
> >   char *p;
> > @@ -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 */
> >   goto is_string;
> >   v = (int)strtol(confptr, (char **)NULL, 10);
> >   yylval.val = v;
> > @@ -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--;
> >   continue;
> >   }
> >   if (d == buf && isspace(*s))
> >
> >
> Hi all,
>
> What are the next steps to push it forward ?
>
> Best regards
> M.K.
>

Testing can't hurt, but it ulimimately boils down to getting an OK
from at least one other developer. Most of us are pretty busy...

        -Otto

Reply | Threaded
Open this post in threaded view
|

Re: [patch] Re: Possible sasyncd memory leak ?

Klemens Nanni-2
In reply to this post by Otto Moerbeek
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.

Reply | Threaded
Open this post in threaded view
|

Re: [patch] Re: Possible sasyncd memory leak ?

Otto Moerbeek
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

Reply | Threaded
Open this post in threaded view
|

Re: [patch] Re: Possible sasyncd memory leak ?

Michał Koc
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.

Reply | Threaded
Open this post in threaded view
|

Re: [patch] Re: Possible sasyncd memory leak ?

Otto Moerbeek
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 -0000 1.19
+++ conf.y 21 Mar 2019 10:51:46 -0000
@@ -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))

Reply | Threaded
Open this post in threaded view
|

Re: [patch] Re: Possible sasyncd memory leak ?

Klemens Nanni-2
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

Reply | Threaded
Open this post in threaded view
|

Re: [patch] Re: Possible sasyncd memory leak ?

Theo de Raadt-2
In reply to this post by Otto Moerbeek
Otto Moerbeek <[hidden email]> 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.y 9 Apr 2017 02:40:24 -0000 1.19
> +++ conf.y 21 Mar 2019 10:51:46 -0000
> @@ -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))
>

Reply | Threaded
Open this post in threaded view
|

Re: [patch] Re: Possible sasyncd memory leak ?

Michał Koc
In reply to this post by Otto Moerbeek
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.







sasyncd.diff (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [patch] Re: Possible sasyncd memory leak ?

Michał Koc
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



sasyncd.diff (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [patch] Re: Possible sasyncd memory leak ?

Michał Koc
In reply to this post by Michał Koc
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.







sasyncd.diff (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [patch] Re: Possible sasyncd memory leak ?

Otto Moerbeek
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.y 9 Apr 2017 02:40:24 -0000 1.19
> +++ conf.y 22 Mar 2019 20:55:21 -0000
> @@ -30,8 +30,10 @@
>  #include <sys/types.h>
>  #include <sys/stat.h>
>  #include <sys/socket.h>
> +#include <arpa/inet.h>
>  #include <ctype.h>
>  #include <fcntl.h>
> +#include <ifaddrs.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> @@ -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
>   {
> @@ -281,6 +276,51 @@ 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 *sa, *peer_sa;
> +
> + sa = host_ip(peer_addr);
> +
> + if(!sa)

I prefer comparing to NULL for pointers.

> + log_msg(2, "config: skip unparseable peer %s", peer_addr);
> +
> + if (sa && getifaddrs(&ifap) == 0) {
> + for (ifa = ifap; ifa && sa; ifa = ifa->ifa_next) {
> + if (!ifa->ifa_addr || ifa->ifa_addr->sa_family != sa->sa_family)

Same here.

> + continue;
> +
> + if (sa->sa_len == ifa->ifa_addr->sa_len && memcmp(sa, ifa->ifa_addr, sa->sa_len) == 0) {
> + log_msg(2, "config: skip local peer %s", peer_addr);
> + free(sa);
> + sa = 0;

Use NULL for pointers.

> + }
> + }
> + freeifaddrs(ifap);
> + }
> +
> + if (sa) {
> +    for (peer = LIST_FIRST(&cfgstate.peerlist); peer && sa;
> + peer = LIST_NEXT(peer, link)) {
> +    peer_sa = host_ip(peer->name);
> +
> +    if (sa->sa_len == peer_sa->sa_len && memcmp(sa, peer_sa, sa->sa_len) == 0) {
> + log_msg(2, "config: skip duplicate peer %s", peer_addr);
> + free(sa);
> + sa = 0;

Use NULL.

> +    }
> +    free(peer_sa);
> +    }
> + }
> +
> + free(sa);
> +
> + return sa != 0;

And here

>  }
>  
>  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 -0000 1.23
> +++ net.c 22 Mar 2019 20:55:21 -0000
> @@ -755,32 +755,26 @@ net_read(struct syncpeer *p, u_int32_t *
>  static int
>  net_set_sa(struct sockaddr *sa, char *name, in_port_t port)
>  {
> - struct sockaddr_in *sin = (struct sockaddr_in *)sa;
> - struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)sa;
> + struct sockaddr *peer_sa = host_ip(name);
>  
> - if (!name) {
> - /* XXX Assume IPv4 */
> - sa->sa_family = AF_INET;
> - sin->sin_port = htons(port);
> - sin->sin_len = sizeof *sin;
> - return 0;
> - }
> + if (peer_sa) {
>  
> - if (inet_pton(AF_INET, name, &sin->sin_addr) == 1) {
> - sa->sa_family = AF_INET;
> - sin->sin_port = htons(port);
> - sin->sin_len = sizeof *sin;
> - return 0;
> - }
> +    memcpy(sa, peer_sa, peer_sa->sa_len);
> +    free(peer_sa);
> +
> +    switch(sa->sa_family) {
> +    case AF_INET:
> + ((struct sockaddr_in *)sa)->sin_port = htons(port);
> + break;
> +
> +    case AF_INET6:
> + ((struct sockaddr_in6 *)sa)->sin6_port = htons(port);
> + break;
> +    }
>  
> - if (inet_pton(AF_INET6, name, &sin6->sin6_addr) == 1) {
> - sa->sa_family = AF_INET6;
> - sin6->sin6_port = htons(port);
> - sin6->sin6_len = sizeof *sin6;
> - return 0;
>   }
>  
> - return -1;
> + return peer_sa == 0 ? -1 : 0;

Use NULL

>  }
>  
>  static void
> @@ -848,4 +842,24 @@ net_check_peers(void *arg)
>  {
>   net_connect();
>   (void)timer_add("peer recheck", 600, net_check_peers, 0);
> +}
> +
> +struct sockaddr *
> +host_ip(const char *hostname) {
> + struct addrinfo *ai = 0;
> + struct sockaddr *sa = 0;
> +
> + if (getaddrinfo(hostname, NULL, NULL, &ai) == 0) {
> +
> + switch (ai->ai_family) {
> + case AF_INET:
> + case AF_INET6:
> + sa = calloc(1, ai->ai_addr->sa_len);
> + memcpy(sa, ai->ai_addr, ai->ai_addr->sa_len);

Always check return value of allocations!
Why use calloc if you are going to overwrite thw whole chunk?

> + break;
> + }
> + freeaddrinfo(ai);
> + }
> +
> + return sa;
>  }
> Index: net.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/sasyncd/net.h,v
> retrieving revision 1.5
> diff -u -p -r1.5 net.h
> --- net.h 2 Jun 2006 20:09:43 -0000 1.5
> +++ net.h 22 Mar 2019 20:55:21 -0000
> @@ -53,6 +53,7 @@ enum CTLTYPE { RESERVED = 0, CTL_STATE,
>  /* net.c */
>  void net_connect(void);
>  void net_disconnect_peer(struct syncpeer *);
> +struct sockaddr *host_ip(const char *);
>  
>  /* net_ctl.c */
>  void net_ctl_handle_msg(struct syncpeer *, u_int8_t *, u_int32_t);

Reply | Threaded
Open this post in threaded view
|

Re: [patch] Re: Possible sasyncd memory leak ?

Michał Koc
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.



sasyncd.diff (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [patch] Re: Possible sasyncd memory leak ?

Otto Moerbeek
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 against NULL. But there are tons of
> pointers matched to 0 or unary operator "!" in the source code, should all
> of them be changed ?

Nah, that's just churn. But for new code, I prefer to explicitly make
a difference between bool, char, int and pointer compares.

        -Otto

> 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 -0000 1.19
> +++ conf.y 23 Mar 2019 16:59:03 -0000
> @@ -30,8 +30,10 @@
>  #include <sys/types.h>
>  #include <sys/stat.h>
>  #include <sys/socket.h>
> +#include <arpa/inet.h>
>  #include <ctype.h>
>  #include <fcntl.h>
> +#include <ifaddrs.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> @@ -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,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) "
>      "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 +276,51 @@ 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 *sa, *peer_sa;
> +
> + sa = host_ip(peer_addr);
> +
> + if(sa == NULL)
> + log_msg(2, "config: skip unparseable peer %s", peer_addr);
> +
> + if (sa != NULL && getifaddrs(&ifap) == 0) {
> + for (ifa = ifap; ifa != NULL && sa != NULL; ifa = ifa->ifa_next) {
> + if (ifa->ifa_addr == NULL || ifa->ifa_addr->sa_family != sa->sa_family)
> + continue;
> +
> + if (sa->sa_len == ifa->ifa_addr->sa_len && memcmp(sa, ifa->ifa_addr, sa->sa_len) == 0) {
> + log_msg(2, "config: skip local peer %s", peer_addr);
> + free(sa);
> + sa = NULL;
> + }
> + }
> + freeifaddrs(ifap);
> + }
> +
> + if (sa != NULL) {
> +    for (peer = LIST_FIRST(&cfgstate.peerlist); peer != NULL && sa != NULL;
> + peer = LIST_NEXT(peer, link)) {
> +    peer_sa = host_ip(peer->name);
> +
> +    if (peer_sa != NULL && sa->sa_len == peer_sa->sa_len && memcmp(sa, peer_sa, sa->sa_len) == 0) {
> + log_msg(2, "config: skip duplicate peer %s", peer_addr);
> + free(sa);
> + sa = NULL;
> +    }
> +    free(peer_sa);
> +    }
> + }
> +
> + free(sa);
> +
> + return sa != NULL;
>  }
>  
>  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 -0000 1.23
> +++ net.c 23 Mar 2019 16:59:03 -0000
> @@ -755,32 +755,26 @@ net_read(struct syncpeer *p, u_int32_t *
>  static int
>  net_set_sa(struct sockaddr *sa, char *name, in_port_t port)
>  {
> - struct sockaddr_in *sin = (struct sockaddr_in *)sa;
> - struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)sa;
> + struct sockaddr *peer_sa = host_ip(name);
>  
> - if (!name) {
> - /* XXX Assume IPv4 */
> - sa->sa_family = AF_INET;
> - sin->sin_port = htons(port);
> - sin->sin_len = sizeof *sin;
> - return 0;
> - }
> + if (peer_sa != NULL) {
>  
> - if (inet_pton(AF_INET, name, &sin->sin_addr) == 1) {
> - sa->sa_family = AF_INET;
> - sin->sin_port = htons(port);
> - sin->sin_len = sizeof *sin;
> - return 0;
> - }
> +    memcpy(sa, peer_sa, peer_sa->sa_len);
> +    free(peer_sa);
> +
> +    switch(sa->sa_family) {
> +    case AF_INET:
> + ((struct sockaddr_in *)sa)->sin_port = htons(port);
> + break;
> +
> +    case AF_INET6:
> + ((struct sockaddr_in6 *)sa)->sin6_port = htons(port);
> + break;
> +    }
>  
> - if (inet_pton(AF_INET6, name, &sin6->sin6_addr) == 1) {
> - sa->sa_family = AF_INET6;
> - sin6->sin6_port = htons(port);
> - sin6->sin6_len = sizeof *sin6;
> - return 0;
>   }
>  
> - return -1;
> + return peer_sa == NULL ? -1 : 0;
>  }
>  
>  static void
> @@ -848,4 +842,25 @@ net_check_peers(void *arg)
>  {
>   net_connect();
>   (void)timer_add("peer recheck", 600, net_check_peers, 0);
> +}
> +
> +struct sockaddr *
> +host_ip(const char *hostname) {
> + struct addrinfo *ai = 0;
> + struct sockaddr *sa = 0;
> +
> + if (getaddrinfo(hostname, NULL, NULL, &ai) == 0) {
> +
> + switch (ai->ai_family) {
> + case AF_INET:
> + case AF_INET6:
> + sa = malloc(ai->ai_addr->sa_len);
> + if(sa != NULL)
> + memcpy(sa, ai->ai_addr, ai->ai_addr->sa_len);
> + break;
> + }
> + freeaddrinfo(ai);
> + }
> +
> + return sa;
>  }
> Index: net.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/sasyncd/net.h,v
> retrieving revision 1.5
> diff -u -p -r1.5 net.h
> --- net.h 2 Jun 2006 20:09:43 -0000 1.5
> +++ net.h 23 Mar 2019 16:59:03 -0000
> @@ -53,6 +53,7 @@ enum CTLTYPE { RESERVED = 0, CTL_STATE,
>  /* net.c */
>  void net_connect(void);
>  void net_disconnect_peer(struct syncpeer *);
> +struct sockaddr *host_ip(const char *);
>  
>  /* net_ctl.c */
>  void net_ctl_handle_msg(struct syncpeer *, u_int8_t *, u_int32_t);

12