[Openvpn-devel,v2] Fix fatal error at switching remotes (#629)

Message ID 20200916141755.1923-1-themiron@yandex-team.ru
State Accepted
Headers show
Series
  • [Openvpn-devel,v2] Fix fatal error at switching remotes (#629)
Related show

Commit Message

Vladislav Grishenko Sept. 16, 2020, 2:17 p.m.
If remote server has been resolved to multiple addresses, at
least one connection attemt has been made and connection to
the last address was skipped by management - resolved earlier
link socket addrinfo objects will not be cleared neither on
instance close nor in the next connection entry loop.
This causes fatal error assert:

    >REMOTE:openvpn.net,1194,udp
    remote ACCEPT
    SUCCESS: remote command succeeded
    >REMOTE:openvpn.net,1194,udp
    remote SKIP
    SUCCESS: remote command succeeded
    >FATAL:Assertion failed at init.c:504 (c->c1.link_socket_addr.current_remote == NULL)

Fix this behaviour by cleaning stale addinfo objects.

v2: better comment placement and too long length fix

Signed-off-by: Vladislav Grishenko <themiron@yandex-team.ru>
---
 src/openvpn/init.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Lev Stipakov Sept. 17, 2020, 6:45 a.m. | #1
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>
Gert Doering Sept. 17, 2020, 8:45 a.m. | #2
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
Vladislav Grishenko Sept. 17, 2020, 8:54 a.m. | #3
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
Gert Doering Sept. 17, 2020, 8:57 a.m. | #4
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
Lev Stipakov Sept. 17, 2020, 9:22 a.m. | #5
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.
Vladislav Grishenko Sept. 17, 2020, 9:33 a.m. | #6
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

Patch

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);