[Openvpn-devel,1/4,v3] pf: clean up temporary files if plugin init fails

Message ID 20171101220342.14648-2-steffan@karger.me
State Accepted
Headers show
Series [Openvpn-devel,1/4,v3] pf: clean up temporary files if plugin init fails | expand

Commit Message

Steffan Karger Nov. 1, 2017, 11:03 a.m. UTC
From: Steffan Karger <steffan.karger@fox-it.com>

close_instance() tries to remove the file in c2.pf.filename, but that only
works if we actually set that if we fail.  So, set that filename as soon
as we know we've created the file.

Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>
---
v2: As suggested by Antionio, get rid of local 'gc' and 'file' vars.
v3: make c->c2.pf.filename const (fixes compile warning)

 src/openvpn/pf.c | 10 ++++------
 src/openvpn/pf.h |  2 +-
 2 files changed, 5 insertions(+), 7 deletions(-)

Comments

Antonio Quartulli Nov. 8, 2017, 4:15 p.m. UTC | #1
On 02/11/17 06:03, Steffan Karger wrote:
> From: Steffan Karger <steffan.karger@fox-it.com>
> 
> close_instance() tries to remove the file in c2.pf.filename, but that only
> works if we actually set that if we fail.  So, set that filename as soon
> as we know we've created the file.
> 
> Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>

Acked-by: Antonio Quartulli <a@unstable.cc>
Gert Doering Nov. 24, 2017, 1:42 a.m. UTC | #2
I like that :-) - less code, less malloc()ing, less gcs around and 
more correct at that.  

I have also briefly reviewed the rest of the series, and everything looks 
good - but since Antonio has already ACKed everything, I leave that honour
to him.

Your patch has been applied to the master branch.

(This is refactoring, according to branch rules it only goes to 2.4 in
exceptionary circumstances, like "another patch depending on it" - but since
tls-crypt-v2 is also intended for master only, this is good enough for now)

commit d2342067d95621b130dd3985a077872b032d53d2
Author: Steffan Karger
Date:   Wed Nov 1 23:03:39 2017 +0100

     pf: clean up temporary files if plugin init fails

     Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>
     Acked-by: Antonio Quartulli <antonio@openvpn.net>
     Message-Id: <20171101220342.14648-2-steffan@karger.me>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15705.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Patch

diff --git a/src/openvpn/pf.c b/src/openvpn/pf.c
index 5cb002bf..e1b5b0e7 100644
--- a/src/openvpn/pf.c
+++ b/src/openvpn/pf.c
@@ -618,19 +618,18 @@  pf_load_from_buffer_list(struct context *c, const struct buffer_list *config)
 void
 pf_init_context(struct context *c)
 {
-    struct gc_arena gc = gc_new();
 #ifdef PLUGIN_PF
     if (plugin_defined(c->plugins, OPENVPN_PLUGIN_ENABLE_PF))
     {
-        const char *pf_file = create_temp_file(c->options.tmp_dir, "pf", &gc);
-        if (pf_file)
+        c->c2.pf.filename = create_temp_file(c->options.tmp_dir, "pf",
+                                             &c->c2.gc);
+        if (c->c2.pf.filename)
         {
-            setenv_str(c->c2.es, "pf_file", pf_file);
+            setenv_str(c->c2.es, "pf_file", c->c2.pf.filename);
 
             if (plugin_call(c->plugins, OPENVPN_PLUGIN_ENABLE_PF, NULL, NULL, c->c2.es) == OPENVPN_PLUGIN_FUNC_SUCCESS)
             {
                 event_timeout_init(&c->c2.pf.reload, 1, now);
-                c->c2.pf.filename = string_alloc(pf_file, &c->c2.gc);
                 c->c2.pf.enabled = true;
 #ifdef ENABLE_DEBUG
                 if (check_debug_level(D_PF_DEBUG))
@@ -658,7 +657,6 @@  pf_init_context(struct context *c)
 #endif
     }
 #endif
-    gc_free(&gc);
 }
 
 void
diff --git a/src/openvpn/pf.h b/src/openvpn/pf.h
index 414c85b8..b839fd2e 100644
--- a/src/openvpn/pf.h
+++ b/src/openvpn/pf.h
@@ -75,7 +75,7 @@  struct pf_context {
     bool enabled;
     struct pf_set *pfs;
 #ifdef PLUGIN_PF
-    char *filename;
+    const char *filename;
     time_t file_last_mod;
     unsigned int n_check_reload;
     struct event_timeout reload;