diff mbox series

[Openvpn-devel] Fix too early argv freeing when registering DNS

Message ID 20201215171600.25534-1-domagoj@pensa.hr
State Accepted
Headers show
Series [Openvpn-devel] Fix too early argv freeing when registering DNS | expand

Commit Message

Domagoj Pensa Dec. 15, 2020, 6:16 a.m. UTC 
When registering DNS on Windows, argv is freed after being used in first
ipconfig command (/flushdns).

Then same argv is used uninitialized in next ipconfig command (/registerdns)
causing heap exception and subprocess crash.

As a consequence second command is never executed and locked netcmd
semaphore is not cleanly released.

Removing argv freeing between ipconfig calls solves the problem.

This issue was introduced in commit 870e240 (argv: do fewer memory
re-allocations). After a quick glance at commit no similar problem was
spotted in rest of the argv related changes.

Signed-off-by: Domagoj Pensa <domagoj@pensa.hr>
---
 src/openvpn/tun.c | 1 -
 1 file changed, 1 deletion(-)

Comments

Gert Doering Dec. 15, 2020, 6:35 a.m. UTC | #1 
Hi,

On Tue, Dec 15, 2020 at 06:16:00PM +0100, Domagoj Pensa wrote:
> When registering DNS on Windows, argv is freed after being used in first
> ipconfig command (/flushdns).
> 
> Then same argv is used uninitialized in next ipconfig command (/registerdns)
> causing heap exception and subprocess crash.
> 
> As a consequence second command is never executed and locked netcmd
> semaphore is not cleanly released.
> 
> Removing argv freeing between ipconfig calls solves the problem.

Oh!  Yes, now with your patch, this is very obvious - there is a trac
ticket (so when I merge this, I'll add the trac ticket number to the
commit message) but it sort of puzzled Selva and me, because the
code "looked sane".

Acked-by: gert@greenie.muc.de

gert
Selva Nair Dec. 15, 2020, 7 a.m. UTC | #2 
Hi

On Tue, Dec 15, 2020 at 12:37 PM Gert Doering <gert@greenie.muc.de> wrote:

> Hi,
>
> On Tue, Dec 15, 2020 at 06:16:00PM +0100, Domagoj Pensa wrote:
> > When registering DNS on Windows, argv is freed after being used in first
> > ipconfig command (/flushdns).
> >
> > Then same argv is used uninitialized in next ipconfig command
> (/registerdns)
> > causing heap exception and subprocess crash.
> >
> > As a consequence second command is never executed and locked netcmd
> > semaphore is not cleanly released.
> >
> > Removing argv freeing between ipconfig calls solves the problem.
>
> Oh!  Yes, now with your patch, this is very obvious - there is a trac
> ticket (so when I merge this, I'll add the trac ticket number to the
> commit message) but it sort of puzzled Selva and me, because the
> code "looked sane".
>

Indeed, I went looking at wrong places.

Thanks,

Selva
<div dir="ltr"><div>Hi</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Dec 15, 2020 at 12:37 PM Gert Doering &lt;<a href="mailto:gert@greenie.muc.de">gert@greenie.muc.de</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi,<br>
<br>
On Tue, Dec 15, 2020 at 06:16:00PM +0100, Domagoj Pensa wrote:<br>
&gt; When registering DNS on Windows, argv is freed after being used in first<br>
&gt; ipconfig command (/flushdns).<br>
&gt; <br>
&gt; Then same argv is used uninitialized in next ipconfig command (/registerdns)<br>
&gt; causing heap exception and subprocess crash.<br>
&gt; <br>
&gt; As a consequence second command is never executed and locked netcmd<br>
&gt; semaphore is not cleanly released.<br>
&gt; <br>
&gt; Removing argv freeing between ipconfig calls solves the problem.<br>
<br>
Oh!  Yes, now with your patch, this is very obvious - there is a trac<br>
ticket (so when I merge this, I&#39;ll add the trac ticket number to the<br>
commit message) but it sort of puzzled Selva and me, because the<br>
code &quot;looked sane&quot;.<br></blockquote><div><br></div><div>Indeed, I went looking at wrong places.</div><div><br></div><div>Thanks,</div><div><br></div><div>Selva</div><div> </div></div></div>
Gert Doering Dec. 15, 2020, 8:21 a.m. UTC | #3 
Thanks.  As already said, all of a sudden it is very obvious
why it is crashing here... somewhat annoying that this wasn't
noticed before 2.5.0 release, though.

I've stared-at-code, and tested this on Win10 (ubuntu 18 / mingw build)
with a config with --register-dns. Without the patch, crash, with the 
patch, it just works.

Testing is not fully straightforward, as you can not "just run" such 
a config, but you need to either run openvpn-gui as admin, or run 
openvpn from a admin-cmd.exe - and it never OOMs for me, just never
proceeeds after "ipconfig.exe /flushdns".

Thanks :-)

Your patch has been applied to the master and release/2.5 branch.

commit ab4688e3bd78d010ccc96adec66ab552bd009328 (master)
commit 2f2df474158b6c24325a47334fc8b5eb77a69b85 (release/2.5)
Author: Domagoj Pensa
Date:   Tue Dec 15 18:16:00 2020 +0100

     Fix too early argv freeing when registering DNS

     Signed-off-by: Domagoj Pensa <domagoj@pensa.hr>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20201215171600.25534-1-domagoj@pensa.hr>
     URL: https://www.mail-archive.com/search?l=mid&q=20201215171600.25534-1-domagoj@pensa.hr
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
Domagoj Pensa Dec. 16, 2020, 9:13 p.m. UTC | #4 
Hi!

You're welcome.
I'm glad that it helps.
Even if it is just one line of code removed. :)

Best regards,
Domagoj

On Tue, Dec 15, 2020 at 08:21:51PM +0100, Gert Doering wrote:
> Thanks.  As already said, all of a sudden it is very obvious
> why it is crashing here... somewhat annoying that this wasn't
> noticed before 2.5.0 release, though.
> 
> I've stared-at-code, and tested this on Win10 (ubuntu 18 / mingw build)
> with a config with --register-dns. Without the patch, crash, with the 
> patch, it just works.
> 
> Testing is not fully straightforward, as you can not "just run" such 
> a config, but you need to either run openvpn-gui as admin, or run 
> openvpn from a admin-cmd.exe - and it never OOMs for me, just never
> proceeeds after "ipconfig.exe /flushdns".
> 
> Thanks :-)
> 
> Your patch has been applied to the master and release/2.5 branch.
> 
> commit ab4688e3bd78d010ccc96adec66ab552bd009328 (master)
> commit 2f2df474158b6c24325a47334fc8b5eb77a69b85 (release/2.5)
> Author: Domagoj Pensa
> Date:   Tue Dec 15 18:16:00 2020 +0100
> 
>      Fix too early argv freeing when registering DNS
> 
>      Signed-off-by: Domagoj Pensa <domagoj@pensa.hr>
>      Acked-by: Gert Doering <gert@greenie.muc.de>
>      Message-Id: <20201215171600.25534-1-domagoj@pensa.hr>
>      URL: https://www.mail-archive.com/search?l=mid&q=20201215171600.25534-1-domagoj@pensa.hr
>      Signed-off-by: Gert Doering <gert@greenie.muc.de>
> 
> 
> --
> kind regards,
> 
> Gert Doering
>
diff mbox series

Patch

diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 400a50ca..2b227bb6 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -5235,7 +5235,6 @@  ipconfig_register_dns(const struct env_set *es)
                 WIN_IPCONFIG_PATH_SUFFIX);
     argv_msg(D_TUNTAP_INFO, &argv);
     openvpn_execve_check(&argv, es, 0, err);
-    argv_free(&argv);
 
     argv_printf(&argv, "%s%s /registerdns",
                 get_win_sys_path(),