[Openvpn-devel,v2] create_temp_file/gen_path: prevent memory leak if gc == NULL

Message ID 20170929161025.13654-1-steffan@karger.me
State Superseded
Headers show
Series
  • [Openvpn-devel,v2] create_temp_file/gen_path: prevent memory leak if gc == NULL
Related show

Commit Message

Steffan Karger Sept. 29, 2017, 4:10 p.m.
If gc == NULL, the data allocated in the alloc_gc_buf() call in
create_temp_file or the string_mod_const call in gen_path would never
be free'd.

These functions are currently never called that way, but let's prevent
future problems.

While touching create_temp_file, also remove the counter variable, which is
never read.

Signed-off-by: Steffan Karger <steffan@karger.me>
---
v2:
 - change create_temp_file to avoid using a struct buffer (simpler)
 - add gc != NULL check for gen_path (avoid similar memleak pitfall)

 src/openvpn/misc.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

Comments

Antonio Quartulli Sept. 30, 2017, 12:48 a.m. | #1
On 30/09/17 00:10, Steffan Karger wrote:
> If gc == NULL, the data allocated in the alloc_gc_buf() call in
> create_temp_file or the string_mod_const call in gen_path would never
> be free'd.
> 
> These functions are currently never called that way, but let's prevent
> future problems.
> 
> While touching create_temp_file, also remove the counter variable, which is
> never read.
> 
> Signed-off-by: Steffan Karger <steffan@karger.me>
> ---
> v2:
>  - change create_temp_file to avoid using a struct buffer (simpler)
>  - add gc != NULL check for gen_path (avoid similar memleak pitfall)
> 
>  src/openvpn/misc.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
> index 90632706..b6f92526 100644
> --- a/src/openvpn/misc.c
> +++ b/src/openvpn/misc.c
> @@ -723,21 +723,20 @@ test_file(const char *filename)
>  const char *
>  create_temp_file(const char *directory, const char *prefix, struct gc_arena *gc)
>  {
> -    static unsigned int counter;
> -    struct buffer fname = alloc_buf_gc(256, gc);
>      int fd;
>      const char *retfname = NULL;
>      unsigned int attempts = 0;
> +    char fname[256] = { 0 };
>  
>      do
>      {
>          ++attempts;
> -        ++counter;
>  
> -        buf_printf(&fname, PACKAGE "_%s_%08lx%08lx.tmp", prefix,
> -                   (unsigned long) get_random(), (unsigned long) get_random());
> +        openvpn_snprintf(fname, sizeof(fname), PACKAGE "_%s_%08lx%08lx.tmp",
> +                         prefix, (unsigned long) get_random(),
> +                         (unsigned long) get_random());

Minor thought: do we want to check if the return value of snprintf is >=
sizeof(fname)? That would tell us if the string did not entirely fit in
the buffer so that we can abort the whole file creation.

Cheers,

>  
> -        retfname = gen_path(directory, BSTR(&fname), gc);
> +        retfname = gen_path(directory, fname, gc);
>          if (!retfname)
>          {
>              msg(M_WARN, "Failed to create temporary filename and path");
> @@ -812,6 +811,12 @@ gen_path(const char *directory, const char *filename, struct gc_arena *gc)
>  #else
>      const int CC_PATH_RESERVED = CC_SLASH;
>  #endif
> +
> +    if (!gc)
> +    {
> +        return NULL; /* Would leak memory otherwise */
> +    }
> +
>      const char *safe_filename = string_mod_const(filename, CC_PRINT, CC_PATH_RESERVED, '_', gc);
>  
>      if (safe_filename
>

Patch

diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
index 90632706..b6f92526 100644
--- a/src/openvpn/misc.c
+++ b/src/openvpn/misc.c
@@ -723,21 +723,20 @@  test_file(const char *filename)
 const char *
 create_temp_file(const char *directory, const char *prefix, struct gc_arena *gc)
 {
-    static unsigned int counter;
-    struct buffer fname = alloc_buf_gc(256, gc);
     int fd;
     const char *retfname = NULL;
     unsigned int attempts = 0;
+    char fname[256] = { 0 };
 
     do
     {
         ++attempts;
-        ++counter;
 
-        buf_printf(&fname, PACKAGE "_%s_%08lx%08lx.tmp", prefix,
-                   (unsigned long) get_random(), (unsigned long) get_random());
+        openvpn_snprintf(fname, sizeof(fname), PACKAGE "_%s_%08lx%08lx.tmp",
+                         prefix, (unsigned long) get_random(),
+                         (unsigned long) get_random());
 
-        retfname = gen_path(directory, BSTR(&fname), gc);
+        retfname = gen_path(directory, fname, gc);
         if (!retfname)
         {
             msg(M_WARN, "Failed to create temporary filename and path");
@@ -812,6 +811,12 @@  gen_path(const char *directory, const char *filename, struct gc_arena *gc)
 #else
     const int CC_PATH_RESERVED = CC_SLASH;
 #endif
+
+    if (!gc)
+    {
+        return NULL; /* Would leak memory otherwise */
+    }
+
     const char *safe_filename = string_mod_const(filename, CC_PRINT, CC_PATH_RESERVED, '_', gc);
 
     if (safe_filename