diff mbox series

[Openvpn-devel] Fix leaks in route.

Message ID 20210714162533.10098-1-david@adalogics.com
State Accepted
Headers show
Series [Openvpn-devel] Fix leaks in route. | expand

Commit Message

david korczynski July 14, 2021, 6:25 a.m. UTC 
Signed-off-by: David Korczynski <david@adalogics.com>
---
 src/openvpn/route.c | 2 ++
 1 file changed, 2 insertions(+)

--
2.17.1

ADA Logics Ltd is registered in England. No: 11624074.
Registered office: 266 Banbury Road, Post Box 292,
OX2 7DL, Oxford, Oxfordshire , United Kingdom

Comments

Antonio Quartulli July 14, 2021, 8:47 p.m. UTC | #1 
Hi,

On 14/07/2021 18:25, David Korczynski wrote:
> Signed-off-by: David Korczynski <david@adalogics.com>

Some commit message is always good - just to give context to the reader
without the need to look at the rest of the code.

But if Gert is fine without it, I am fine too.

> ---
>  src/openvpn/route.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/openvpn/route.c b/src/openvpn/route.c
> index 56fa7717..e429e8c0 100644
> --- a/src/openvpn/route.c
> +++ b/src/openvpn/route.c
> @@ -1584,6 +1584,7 @@ add_route(struct route_ipv4 *r,
> 
>      if (!(r->flags & RT_DEFINED))
>      {
> +        argv_free(&argv);
>          return;
>      }
> 
> @@ -1891,6 +1892,7 @@ add_route_ipv6(struct route_ipv6 *r6, const struct tuntap *tt,
> 
>      if (!(r6->flags & RT_DEFINED) )
>      {
> +        argv_free(&argv);
>          return;
>      }
> 

I agree with Gert that not having RT_DEFINED is indeed impossible with
the current code, because that flag is always set when creating a route.
However, this function does not know about it and it is correct to keep
this check.

Since we keep this check, we indeed need to properly bail out and do not
leak.

Thanks for noticing and fixing this!

Acked-by: Antonio Quartulli <antonio@openvpn.net>
Gert Doering July 14, 2021, 11:10 p.m. UTC | #2 
Hi,

On Thu, Jul 15, 2021 at 08:47:13AM +0200, Antonio Quartulli wrote:
> On 14/07/2021 18:25, David Korczynski wrote:
> > Signed-off-by: David Korczynski <david@adalogics.com>
> 
> Some commit message is always good - just to give context to the reader
> without the need to look at the rest of the code.
> 
> But if Gert is fine without it, I am fine too.

I am fine, in this case, as there is not really much to say here - maybe
add the explanation "we assume that RT_DEFINED is always set, but still,
if it is not, we must not leak memory".

I'll merge later today :-)

gert
david korczynski July 19, 2021, 5:29 a.m. UTC | #3 
Gert and Antonio, I am not sure if you would want me to change the
commit message or not. Let me know if you want me to change it and then
I will do that.

On 15/07/2021 10:10, Gert Doering wrote:
> ADA Logics Ltd is registered in England. No: 11624074.
> Registered office: 266 Banbury Road, Post Box 292,
> OX2 7DL, Oxford, Oxfordshire , United Kingdom
ADA Logics Ltd is registered in England. No: 11624074.
Registered office: 266 Banbury Road, Post Box 292,
OX2 7DL, Oxford, Oxfordshire , United Kingdom
Gert Doering July 19, 2021, 6:11 a.m. UTC | #4 
Hi,

On Mon, Jul 19, 2021 at 04:29:32PM +0100, david korczynski wrote:
> Gert and Antonio, I am not sure if you would want me to change the
> commit message or not. Let me know if you want me to change it and then
> I will do that.

Nah, I'll adjust the commit message when commiting.

(Our rule is: code is never changed compared to what is on the list,
whitespace/comments in code can be adjusted if it makes sense and the
author is OK, and commit messages can be adjusted if the commit itself
is good but there is grammar errors, a link to trac missing, ...)

gert
david korczynski July 19, 2021, 6:24 a.m. UTC | #5 
Sounds good, and the rule of committing code sounds good to me! I will
be looking to providing more patches based on what the fuzzing finds in
the near future.

On 19/07/2021 17:11, Gert Doering wrote:
> ADA Logics Ltd is registered in England. No: 11624074.
> Registered office: 266 Banbury Road, Post Box 292,
> OX2 7DL, Oxford, Oxfordshire , United Kingdom
ADA Logics Ltd is registered in England. No: 11624074.
Registered office: 266 Banbury Road, Post Box 292,
OX2 7DL, Oxford, Oxfordshire , United Kingdom
Gert Doering July 27, 2021, 4:24 a.m. UTC | #6 
Thanks for your patch.  I have not seriously tested (just a basic
compile test) as it is "obviously correct" for that special case.

I have extended the commit message a bit to explain why the change
was done (this is what Antonio was talking about - "help the casual
reader who wants to understand this change in +3 years").

Your patch has been applied to the master, release/2.5 and release/2.4 
branch (bugfixes get backported to all supported branches).

commit a11bea18b1c93b260352ec505db15be0ec9431ee (master)
commit 5e88f131b55af96782532f3d1e086de4e17260fe (release/2.5)
commit f2f64e058b819d07da9125824717e4bc4d8f9a9d (HEAD -> release/2.4)
Author: David Korczynski
Date:   Wed Jul 14 17:25:33 2021 +0100

     Fix argv leaks in add_route() and add_route_ipv6()

     Signed-off-by: David Korczynski <david@adalogics.com>
     Acked-by: Antonio Quartulli <antonio@openvpn.net>
     Message-Id: <20210714162533.10098-1-david@adalogics.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg22637.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/route.c b/src/openvpn/route.c
index 56fa7717..e429e8c0 100644
--- a/src/openvpn/route.c
+++ b/src/openvpn/route.c
@@ -1584,6 +1584,7 @@  add_route(struct route_ipv4 *r,

     if (!(r->flags & RT_DEFINED))
     {
+        argv_free(&argv);
         return;
     }

@@ -1891,6 +1892,7 @@  add_route_ipv6(struct route_ipv6 *r6, const struct tuntap *tt,

     if (!(r6->flags & RT_DEFINED) )
     {
+        argv_free(&argv);
         return;
     }