[Openvpn-devel,v2] Remove --writepid file on program exit.

Message ID 20200707084220.45753-1-gert@greenie.muc.de
State Accepted
Headers show
Series
  • [Openvpn-devel,v2] Remove --writepid file on program exit.
Related show

Commit Message

Gert Doering July 7, 2020, 8:42 a.m.
For whatever reason, we never removed the pid file on program exit.

Not only this is unclean, but it also makes testing for "I want this
test case to FAIL" in t_client.sh more annoying to code for "is the
OpenVPN process still around?"...

Do not unlink the file if chroot() is active (might be outside the
chroot arena - testing for realpath etc. is left for someone else).

Signed-off-by: Gert Doering <gert@greenie.muc.de>

--
v2: make this work on M_FATAL exit, by unlinking from openvpn_exit() in
error.h - this requires moving write_pid() to init.c so module hierarchy
is maintained and introducing a static variable to save the PID file
name (otherwise it is no longer available when the top level GC is gone).
---
 src/openvpn/error.c   |  1 +
 src/openvpn/init.c    | 42 ++++++++++++++++++++++++++++++++++++++++++
 src/openvpn/init.h    |  3 +++
 src/openvpn/openvpn.c | 24 +-----------------------
 4 files changed, 47 insertions(+), 23 deletions(-)

Comments

Antonio Quartulli July 7, 2020, 9:45 a.m. | #1
Hi,

On 07/07/2020 10:42, Gert Doering wrote:
> For whatever reason, we never removed the pid file on program exit.
> 
> Not only this is unclean, but it also makes testing for "I want this
> test case to FAIL" in t_client.sh more annoying to code for "is the
> OpenVPN process still around?"...
> 
> Do not unlink the file if chroot() is active (might be outside the
> chroot arena - testing for realpath etc. is left for someone else).
> 
> Signed-off-by: Gert Doering <gert@greenie.muc.de>
> 
> --
> v2: make this work on M_FATAL exit, by unlinking from openvpn_exit() in
> error.h - this requires moving write_pid() to init.c so module hierarchy
> is maintained and introducing a static variable to save the PID file
> name (otherwise it is no longer available when the top level GC is gone).
> ---
>  src/openvpn/error.c   |  1 +
>  src/openvpn/init.c    | 42 ++++++++++++++++++++++++++++++++++++++++++
>  src/openvpn/init.h    |  3 +++
>  src/openvpn/openvpn.c | 24 +-----------------------
>  4 files changed, 47 insertions(+), 23 deletions(-)
> 
> diff --git a/src/openvpn/error.c b/src/openvpn/error.c
> index ad4f0ef4..d6247fec 100644
> --- a/src/openvpn/error.c
> +++ b/src/openvpn/error.c
> @@ -743,6 +743,7 @@ openvpn_exit(const int status)
>  #ifdef _WIN32
>          uninit_win32();
>  #endif
> +        remove_pid_file();
>  
>          close_syslog();
>  
> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
> index dd1747f3..cb850bc0 100644
> --- a/src/openvpn/init.c
> +++ b/src/openvpn/init.c
> @@ -58,6 +58,7 @@
>  
>  
>  static struct context *static_context; /* GLOBAL */
> +static const char *saved_pid_file_name; /* GLOBAL */
>  
>  /*
>   * Crypto initialization flags
> @@ -4687,6 +4688,47 @@ close_context(struct context *c, int sig, unsigned int flags)
>      }
>  }
>  
> +/* Write our PID to a file */
> +void
> +write_pid_file(const char *filename, const char *chroot_dir)
> +{
> +    if (filename)
> +    {
> +        unsigned int pid = 0;
> +        FILE *fp = platform_fopen(filename, "w");
> +        if (!fp)
> +        {
> +            msg(M_ERR, "Open error on pid file %s", filename);
> +            return;
> +        }
> +
> +        pid = platform_getpid();
> +        fprintf(fp, "%u\n", pid);
> +        if (fclose(fp))
> +        {
> +            msg(M_ERR, "Close error on pid file %s", filename);
> +        }
> +
> +        /* remember file name so it can be deleted "out of context" later */
> +	/* (the chroot case is more complex and not handled today) */


This comment is indented using a tab, instead of the proper amount of
spaces.


> +        if (!chroot_dir)
> +        {
> +            saved_pid_file_name = strdup(filename);
> +        }
> +    }
> +}
> +
> +/* remove PID file on exit, called from openvpn_exit() */
> +void
> +remove_pid_file(void)
> +{
> +    if (saved_pid_file_name)
> +    {
> +        platform_unlink(saved_pid_file_name);
> +    }
> +}
> +
> +
>  /*
>   * Do a loopback test
>   * on the crypto subsystem.
> diff --git a/src/openvpn/init.h b/src/openvpn/init.h
> index 0e6258f0..a2fdccd3 100644
> --- a/src/openvpn/init.h
> +++ b/src/openvpn/init.h
> @@ -143,4 +143,7 @@ void open_plugins(struct context *c, const bool import_options, int init_point);
>  
>  void tun_abort(void);
>  
> +void write_pid_file(const char *filename, const char *chroot_dir);
> +void remove_pid_file(void);
> +
>  #endif /* ifndef INIT_H */
> diff --git a/src/openvpn/openvpn.c b/src/openvpn/openvpn.c
> index dc7001dc..857c5faa 100644
> --- a/src/openvpn/openvpn.c
> +++ b/src/openvpn/openvpn.c
> @@ -46,28 +46,6 @@ process_signal_p2p(struct context *c)
>      return process_signal(c);
>  }
>  
> -/* Write our PID to a file */
> -static void
> -write_pid(const char *filename)
> -{
> -    if (filename)
> -    {
> -        unsigned int pid = 0;
> -        FILE *fp = platform_fopen(filename, "w");
> -        if (!fp)
> -        {
> -            msg(M_ERR, "Open error on pid file %s", filename);
> -        }
> -
> -        pid = platform_getpid();
> -        fprintf(fp, "%u\n", pid);
> -        if (fclose(fp))
> -        {
> -            msg(M_ERR, "Close error on pid file %s", filename);
> -        }
> -    }
> -}
> -
>  
>  /**************************************************************************/
>  /**
> @@ -274,7 +252,7 @@ openvpn_main(int argc, char *argv[])
>              if (c.first_time)
>              {
>                  c.did_we_daemonize = possibly_become_daemon(&c.options);
> -                write_pid(c.options.writepid);
> +                write_pid_file(c.options.writepid, c.options.chroot_dir);
>              }
>  
>  #ifdef ENABLE_MANAGEMENT
> 

Other than the comment nitpick, everything looks good. The patch has
been tested and works as expected.

The pidfile is always removed, regardless of the openvpn exist status.

Acked-by: Antonio Quartulli <a@unstable.cc>
Gert Doering July 7, 2020, 9:54 a.m. | #2
Patch has been applied to the master branch.

Tab has been expanded to pacify the whitespace dragon.

commit 008ec688d06101c0307e6d17a0239b134355dca4
Author: Gert Doering
Date:   Tue Jul 7 10:42:20 2020 +0200

     Remove --writepid file on program exit.

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


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/error.c b/src/openvpn/error.c
index ad4f0ef4..d6247fec 100644
--- a/src/openvpn/error.c
+++ b/src/openvpn/error.c
@@ -743,6 +743,7 @@  openvpn_exit(const int status)
 #ifdef _WIN32
         uninit_win32();
 #endif
+        remove_pid_file();
 
         close_syslog();
 
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index dd1747f3..cb850bc0 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -58,6 +58,7 @@ 
 
 
 static struct context *static_context; /* GLOBAL */
+static const char *saved_pid_file_name; /* GLOBAL */
 
 /*
  * Crypto initialization flags
@@ -4687,6 +4688,47 @@  close_context(struct context *c, int sig, unsigned int flags)
     }
 }
 
+/* Write our PID to a file */
+void
+write_pid_file(const char *filename, const char *chroot_dir)
+{
+    if (filename)
+    {
+        unsigned int pid = 0;
+        FILE *fp = platform_fopen(filename, "w");
+        if (!fp)
+        {
+            msg(M_ERR, "Open error on pid file %s", filename);
+            return;
+        }
+
+        pid = platform_getpid();
+        fprintf(fp, "%u\n", pid);
+        if (fclose(fp))
+        {
+            msg(M_ERR, "Close error on pid file %s", filename);
+        }
+
+        /* remember file name so it can be deleted "out of context" later */
+	/* (the chroot case is more complex and not handled today) */
+        if (!chroot_dir)
+        {
+            saved_pid_file_name = strdup(filename);
+        }
+    }
+}
+
+/* remove PID file on exit, called from openvpn_exit() */
+void
+remove_pid_file(void)
+{
+    if (saved_pid_file_name)
+    {
+        platform_unlink(saved_pid_file_name);
+    }
+}
+
+
 /*
  * Do a loopback test
  * on the crypto subsystem.
diff --git a/src/openvpn/init.h b/src/openvpn/init.h
index 0e6258f0..a2fdccd3 100644
--- a/src/openvpn/init.h
+++ b/src/openvpn/init.h
@@ -143,4 +143,7 @@  void open_plugins(struct context *c, const bool import_options, int init_point);
 
 void tun_abort(void);
 
+void write_pid_file(const char *filename, const char *chroot_dir);
+void remove_pid_file(void);
+
 #endif /* ifndef INIT_H */
diff --git a/src/openvpn/openvpn.c b/src/openvpn/openvpn.c
index dc7001dc..857c5faa 100644
--- a/src/openvpn/openvpn.c
+++ b/src/openvpn/openvpn.c
@@ -46,28 +46,6 @@  process_signal_p2p(struct context *c)
     return process_signal(c);
 }
 
-/* Write our PID to a file */
-static void
-write_pid(const char *filename)
-{
-    if (filename)
-    {
-        unsigned int pid = 0;
-        FILE *fp = platform_fopen(filename, "w");
-        if (!fp)
-        {
-            msg(M_ERR, "Open error on pid file %s", filename);
-        }
-
-        pid = platform_getpid();
-        fprintf(fp, "%u\n", pid);
-        if (fclose(fp))
-        {
-            msg(M_ERR, "Close error on pid file %s", filename);
-        }
-    }
-}
-
 
 /**************************************************************************/
 /**
@@ -274,7 +252,7 @@  openvpn_main(int argc, char *argv[])
             if (c.first_time)
             {
                 c.did_we_daemonize = possibly_become_daemon(&c.options);
-                write_pid(c.options.writepid);
+                write_pid_file(c.options.writepid, c.options.chroot_dir);
             }
 
 #ifdef ENABLE_MANAGEMENT