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

Message ID 20170928155446.32382-1-steffan@karger.me
State Superseded
Headers show
Series
  • Untitled series #1
Related show

Commit Message

Steffan Karger Sept. 28, 2017, 3:54 p.m.
If gc == NULL, the data allocated in the alloc_gc_buf() call would never
be free'd.  The function is never called that way, but let's prevent
future problems.

Signed-off-by: Steffan Karger <steffan@karger.me>
---
 src/openvpn/misc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Antonio Quartulli Sept. 29, 2017, 2:51 a.m. | #1
Hi,

On 28/09/17 23:54, Steffan Karger wrote:
> If gc == NULL, the data allocated in the alloc_gc_buf() call would never
> be free'd.  The function is never called that way, but let's prevent
> future problems.

Same problem exists with the invocation of gen_path() a few lines below:
it invokes string_mod_const() that invokes string_alloc(.., gc).

Would you fix that as well? Under the same umbrella patch maybe?

> 
> Signed-off-by: Steffan Karger <steffan@karger.me>
> ---
>  src/openvpn/misc.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
> index 90632706..86f80a6a 100644
> --- a/src/openvpn/misc.c
> +++ b/src/openvpn/misc.c
> @@ -724,10 +724,13 @@ 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;
> +    struct buffer fname = (struct buffer) { 0 };
> +    char buf_data[256] = { 0 };
> +
> +    buf_set_write(&fname, buf_data, sizeof(buf_data));

This buffer is really only used to create a filename and then it's
passed to gen_path().

Is it really worth creating a struct buffer, embedding a string and then
extrapolating the string again to call gen_path()?

How about directly using the string directly?

The only argument against this approach I can see is that buf_printf()
does all the checks for us, while if we switch to a snprintf() we need
to add a one-line-check to ensure the resulting string could fit fname.

What do you think?
I am just trying to simplify the code while at it ..


Cheers,

Patch

diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
index 90632706..86f80a6a 100644
--- a/src/openvpn/misc.c
+++ b/src/openvpn/misc.c
@@ -724,10 +724,13 @@  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;
+    struct buffer fname = (struct buffer) { 0 };
+    char buf_data[256] = { 0 };
+
+    buf_set_write(&fname, buf_data, sizeof(buf_data));
 
     do
     {