Re: [Openvpn-devel] [PATCH] Fix temporary file leak

[David Sommerseth]

On 14/10/14 18:15, David Sommerseth wrote:
> On 12/10/14 21:59, Samuel Thibault wrote:
>> David Sommerseth, le Fri 10 Oct 2014 12:13:42 +0200, a écrit :
>>> I think it would be better to move the unlink() code from
>>> multi_client_connect_post() into multi_connection_established(), where
>>> these temp files are created.  This makes the code clearer and easier to
>>> understand.
>>
>> Right, how about this?
>>
>> Samuel
> 
> Okay, so I messed up the push.  But it's fixed now, and it really got
> pushed out this time :)
> 
> Your patch has been applied to the master branch.
> 
> commit 7da9d40243e0743e2d050ceb6ae34e467dd58973
> Author: Samuel Thibault
> Date:   Thu Oct 9 23:40:49 2014 +0200
> 
>  Ensure that client-connect files are always deleted
> 
>  Signed-off-by: David Sommerseth 
>  Acked-by: David Sommerseth 
>  Message-Id: 20141012195919.GU3738@type
>  URL:
> 


We decided to include this in the coming 2.3 release as well.  So the
patch can be found in the release/2.3 branch as commit 10cbaea91d7d3774
as well.


-- 
kind regards,

David Sommerseth



signature.asc
Description: OpenPGP digital signature

Re: [Openvpn-devel] [PATCH] Fix temporary file leak

[David Sommerseth]

On 12/10/14 21:59, Samuel Thibault wrote:
> David Sommerseth, le Fri 10 Oct 2014 12:13:42 +0200, a écrit :
>> I think it would be better to move the unlink() code from
>> multi_client_connect_post() into multi_connection_established(), where
>> these temp files are created.  This makes the code clearer and easier to
>> understand.
> 
> Right, how about this?
> 
> Samuel

Okay, so I messed up the push.  But it's fixed now, and it really got
pushed out this time :)

Your patch has been applied to the master branch.

commit 7da9d40243e0743e2d050ceb6ae34e467dd58973
Author: Samuel Thibault
List-Post: openvpn-devel@lists.sourceforge.net
Date:   Thu Oct 9 23:40:49 2014 +0200

 Ensure that client-connect files are always deleted

 Signed-off-by: David Sommerseth 
 Acked-by: David Sommerseth 
 Message-Id: 20141012195919.GU3738@type
 URL:



-- 
kind regards,

David Sommerseth




signature.asc
Description: OpenPGP digital signature

Re: [Openvpn-devel] [PATCH] Fix temporary file leak

[Samuel Thibault]

David Sommerseth, le Fri 10 Oct 2014 12:13:42 +0200, a écrit :
> I think it would be better to move the unlink() code from
> multi_client_connect_post() into multi_connection_established(), where
> these temp files are created.  This makes the code clearer and easier to
> understand.

Right, how about this?

Samuel

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 5910154..25e7384 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -1459,10 +1465,6 @@ multi_client_connect_post (struct multi_context *m,
 option_types_found,
 mi->context.c2.es);
 
-  if (!platform_unlink (dc_file))
-   msg (D_MULTI_ERRORS, "MULTI: problem deleting temporary file: %s",
-dc_file);
-
   /*
* If the --client-connect script generates a config file
* with an --ifconfig-push directive, it will override any
@@ -1705,6 +1707,11 @@ multi_connection_established (struct multi_context *m, 
struct multi_instance *mi
  multi_client_connect_post (m, mi, dc_file, 
option_permissions_mask, _types_found);
  ++cc_succeeded_count;
}
+
+ if (!platform_unlink (dc_file))
+   msg (D_MULTI_ERRORS, "MULTI: problem deleting temporary file: %s",
+dc_file);
+
 script_depr_failed:
  argv_reset ();
}
@@ -1758,6 +1765,11 @@ multi_connection_established (struct multi_context *m, 
struct multi_instance *mi
}
  else
cc_succeeded = false;
+
+ if (!platform_unlink (dc_file))
+   msg (D_MULTI_ERRORS, "MULTI: problem deleting temporary file: %s",
+dc_file);
+
 script_failed:
  argv_reset ();
}

Re: [Openvpn-devel] [PATCH] Fix temporary file leak

[David Sommerseth]

On 09/10/14 23:40, Samuel Thibault wrote:
> Hello,
> 
> Our openvpn server got out of free inodes in /tmp, making it quite
> completely nonworking.  This is due to some codepath in multi.c which
> does not remove its temporary file (when a plugin callback returns an
> error, or a client-connect script returns an error).  Please see the
> attached patch which fixes this.
> 
> Samuel
> 
> 
> patch
> 
> 
> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
> index 5910154..d0ed147 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -1699,6 +1705,9 @@ multi_connection_established (struct multi_context *m, 
> struct multi_instance *mi
>   {
> msg (M_WARN, "WARNING: client-connect plugin call failed");
> cc_succeeded = false;
> +   if (!platform_unlink (dc_file))
> + msg (D_MULTI_ERRORS, "MULTI: problem deleting temporary file: 
> %s",
> +  dc_file);
>   }
> else
>   {
> @@ -1757,7 +1766,12 @@ multi_connection_established (struct multi_context *m, 
> struct multi_instance *mi
> ++cc_succeeded_count;
>   }
> else
> - cc_succeeded = false;
> + {
> +   cc_succeeded = false;
> +   if (!platform_unlink (dc_file))
> + msg (D_MULTI_ERRORS, "MULTI: problem deleting temporary file: 
> %s",
> +  dc_file);
> + }
>  script_failed:
> argv_reset ();
>   }
> 

Good catch!  But I'm not convinced about this approach, from a code
quality perspective.  Now we have the unlinking of these temp files two
places.  In multi_client_connect_post() if the plug-in(s) didn't fail,
or in multi_connection_established() if the plug-in failed.

I think it would be better to move the unlink() code from
multi_client_connect_post() into multi_connection_established(), where
these temp files are created.  This makes the code clearer and easier to
understand.  And you can have a label and goto statement in the error
section, similar to what's already done (look for script_depr_failed:
and script_failed:).


-- 
kind regards,

David Sommerseth




signature.asc
Description: OpenPGP digital signature

[Openvpn-devel] [PATCH] Fix temporary file leak

[Samuel Thibault]

Hello,

Our openvpn server got out of free inodes in /tmp, making it quite
completely nonworking.  This is due to some codepath in multi.c which
does not remove its temporary file (when a plugin callback returns an
error, or a client-connect script returns an error).  Please see the
attached patch which fixes this.

Samuel
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 5910154..d0ed147 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -1699,6 +1705,9 @@ multi_connection_established (struct multi_context *m, 
struct multi_instance *mi
{
  msg (M_WARN, "WARNING: client-connect plugin call failed");
  cc_succeeded = false;
+ if (!platform_unlink (dc_file))
+   msg (D_MULTI_ERRORS, "MULTI: problem deleting temporary file: 
%s",
+dc_file);
}
  else
{
@@ -1757,7 +1766,12 @@ multi_connection_established (struct multi_context *m, 
struct multi_instance *mi
  ++cc_succeeded_count;
}
  else
-   cc_succeeded = false;
+   {
+ cc_succeeded = false;
+ if (!platform_unlink (dc_file))
+   msg (D_MULTI_ERRORS, "MULTI: problem deleting temporary file: 
%s",
+dc_file);
+   }
 script_failed:
  argv_reset ();
}