| Message ID | 20200916141755.1923-1-themiron@yandex-team.ru |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel,v2] Fix fatal error at switching remotes (#629) | expand |
Hi, > If remote server has been resolved to multiple addresses, at > least one connection attemt typ0 > Fix this behaviour by cleaning stale addinfo objects. Stared at code and it looks fine - same cleanup logic as in do_close_link_socket(). Built and tested with MSVC - reproduced assert without patch and no assert with patch. Acked-by: Lev Stipakov <lstipakov@gmail.com>
Your patch has been applied to the master, release/2.5 and release/2.4 branch
(bugfix).
I have fixed a few "addinfo" occurances and re-wrapped the comment
slightly. Not checked the actual code, just ran a t_client test on
2.4 "to be sure".
As Arne wrote there is a "fix for real" dangling here... :-)
commit 3ad86c2534a92af137809b6d446d570193e6d01f (master)
commit 6554025a422d3d7e5465bcbfad34fa3e196b53b0 (release/2.5)
commit 7fdcd286a15fb4f64e979c4fdbf52223d4bdede0 (release/2.4)
Author: Vladislav Grishenko
Date: Wed Sep 16 19:17:55 2020 +0500
Fix fatal error at switching remotes (#629)
Signed-off-by: Vladislav Grishenko <themiron@yandex-team.ru>
Acked-by: Lev Stipakov <lstipakov@gmail.com>
Message-Id: <20200916141755.1923-1-themiron@yandex-team.ru>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg21019.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
Thank you a lot, That "fix for real" is about persist_remote_ip option as far as I understand, not directly related to this fatal assert fix. -- Best Regards, Vladislav Grishenko > -----Original Message----- > From: Gert Doering <gert@greenie.muc.de> > Sent: Thursday, September 17, 2020 1:46 PM > To: Vladislav Grishenko <themiron@yandex-team.ru> > Cc: openvpn-devel@lists.sourceforge.net > Subject: [PATCH applied] Re: Fix fatal error at switching remotes (#629) > > Your patch has been applied to the master, release/2.5 and release/2.4 branch > (bugfix). > > I have fixed a few "addinfo" occurances and re-wrapped the comment slightly. > Not checked the actual code, just ran a t_client test on > 2.4 "to be sure". > > As Arne wrote there is a "fix for real" dangling here... :-) > > commit 3ad86c2534a92af137809b6d446d570193e6d01f (master) commit > 6554025a422d3d7e5465bcbfad34fa3e196b53b0 (release/2.5) commit > 7fdcd286a15fb4f64e979c4fdbf52223d4bdede0 (release/2.4) > Author: Vladislav Grishenko > Date: Wed Sep 16 19:17:55 2020 +0500 > > Fix fatal error at switching remotes (#629) > > Signed-off-by: Vladislav Grishenko <themiron@yandex-team.ru> > Acked-by: Lev Stipakov <lstipakov@gmail.com> > Message-Id: <20200916141755.1923-1-themiron@yandex-team.ru> > URL: https://www.mail-archive.com/openvpn- > devel@lists.sourceforge.net/msg21019.html > Signed-off-by: Gert Doering <gert@greenie.muc.de> > > > -- > kind regards, > > Gert Doering
Hi, On Thu, Sep 17, 2020 at 01:54:39PM +0500, Vladislav Grishenko wrote: > Thank you a lot, Not needed :-) > That "fix for real" is about persist_remote_ip option as far as I > understand, not directly related to this fatal assert fix. Well, the whole preresolve / connection entry "complex" is old and has been extended and updated a few times, and your SVR patch also builds on top of that. At some point, refactoring is needed... (We have some other thing to consider which is even more intrusive - when we reconnect to a different IP address, and that new IP address is currently routed into the tunnel, we need to set up new /32 host routes before moving to a new server can work... openvpn3, as I understand, sets up "all host routes!" right at the start, but that might or might not be the best solution either) gert
Hi,
> openvpn3, as I understand, sets up "all host routes!" right at the start
It depends on how openvpn3 library is used.
OpenVPN3 Linux client adds bypass route for the specific remote
just before connection attempt. Same for our Connect Windows / Mac clients,
which are partially closed-source, but this specific logic is part of
opensource library and "agent" (windows agent is opensourced,
mac agent is still closed-source) - a component similar to "iservice".
IIRC, "test" client (cli) indeed adds all routes from the beginning.
Hi, Gert > > That "fix for real" is about persist_remote_ip option as far as I > > understand, not directly related to this fatal assert fix. > > Well, the whole preresolve / connection entry "complex" is old and has been > extended and updated a few times, and your SVR patch also builds on top of > that. That's true, I hit this assert for SRV initially, 'coz same advancing logic was used, v5 version is upcoming following this commit. > At some point, refactoring is needed... > (We have some other thing to consider which is even more intrusive - when we > reconnect to a different IP address, and that new IP address is currently routed > into the tunnel, we need to set up new /32 host routes before moving to a new > server can work... openvpn3, as I understand, sets up "all host routes!" right at > the start, but that might or might not be the best solution either) New address is being handled in disconnected (yet) state, so tunnel routes should not be active, since 2.x supports at most one tunnel active. While this is preserved, /32 host route can be made anytime between resolved state and connection attempt, sounds not so intrusive, if I got you right. Meanwhile, "--persist-remote-ip" documented as "Preserve most recently authenticated remote IP address and port number across SIGUSR1 and --ping-restart". Current implementation doesn't follow it precisely, instead it does "Preserve most recently authenticated remote host name and port...", if that remote name resolves into multiple addresses - they will be still iterated. Guess, this is what was meant by "fix by real" > > gert > -- > "If was one thing all people took for granted, was conviction that if you feed > honest figures into a computer, honest figures come out. Never doubted it > myself till I met a computer with a sense of humor." > Robert A. Heinlein, The Moon is a Harsh Mistress > > Gert Doering - Munich, Germany gert@greenie.muc.de
diff --git a/src/openvpn/init.c b/src/openvpn/init.c index a785934a..8fc79638 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -500,6 +500,17 @@ next_connection_entry(struct context *c) */ if (!c->options.persist_remote_ip) { + /* Connection entry addinfo objects might have been resolved + * earlier but the entry itself might have been skipped by + * management on the previous loop. + * If so, clear the addrinfo objects as close_instance does + */ + if (c->c1.link_socket_addr.remote_list) + { + clear_remote_addrlist(&c->c1.link_socket_addr, + !c->options.resolve_in_advance); + } + /* close_instance should have cleared the addrinfo objects */ ASSERT(c->c1.link_socket_addr.current_remote == NULL); ASSERT(c->c1.link_socket_addr.remote_list == NULL);