Add missing strdup() return value checks

PATCH In review Steffan Karger 3 participants 5 messages Aug 25, 2015 – Oct 4, 2015

Share & tools

Steffan Karger

author

2015-08-25 22:29 UTC #0 lore raw

PATCH 1/1 Add missing strdup() return value checks

As reported by Bill Parker in trac #600, strdup() return values are not always correctly checked for failed allocations. This patch adds missing checks.

Note that in misc.c and options.c, the check is after the dirname() or basename() call, because these can deal with NULL params and we need to keep MSVC happy with its ancient no-declaration-after-statement policy.

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/cryptoapi.c    | 1 +
 src/openvpn/init.c         | 1 +
 src/openvpn/misc.c         | 1 +
 src/openvpn/options.c      | 1 +
 src/openvpn/ssl_polarssl.c | 1 +
 5 files changed, 5 insertions(+)

src/openvpn/cryptoapi.c +1 -0

index b7fc11e..62f30be 100644
@@ -127,6 +127,7 @@ static char *ms_error_text(DWORD ms_err)
 		    break;
 	    }
     }
+    check_malloc_return(rv);
     return rv;
 }

src/openvpn/init.c +1 -0

index b7c153b..0809cc7 100644
@@ -817,6 +817,7 @@ init_options_dev (struct options *options)
 {
   if (!options->dev && options->dev_node) {
     char *dev_node = strdup(options->dev_node); /* POSIX basename() implementaions may modify its arguments */
+    check_malloc_return(dev_node);
     options->dev = basename (dev_node);
   }
 }

src/openvpn/misc.c +1 -0

index a5bad0d..b543b65 100644
@@ -1608,6 +1608,7 @@ argv_extract_cmd_name (const char *path)
     {
       char *path_cp = strdup(path); /* POSIX basename() implementaions may modify its arguments */
       const char *bn = basename (path_cp);
+      check_malloc_return(path_cp);
       if (bn)
 	{
 	  char *ret = string_alloc (bn, NULL);

src/openvpn/options.c +1 -0

index 2784580..dba7c9d 100644
@@ -2588,6 +2588,7 @@ check_file_access(const int type, const char *file, const int mode, const char *
     {
       char *fullpath = strdup(file);  /* POSIX dirname() implementaion may modify its arguments */
       char *dirpath = dirname(fullpath);
+      check_malloc_return(fullpath);
 
       if (platform_access (dirpath, mode|X_OK) != 0)
           errcode = errno;

src/openvpn/ssl_polarssl.c +1 -0

index 3fc811e..673dbbe 100644
@@ -198,6 +198,7 @@ tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers)
   /* Parse allowed ciphers, getting IDs */
   i = 0;
   tmp_ciphers_orig = tmp_ciphers = strdup(ciphers);
+  check_malloc_return(tmp_ciphers);
 
   token = strtok (tmp_ciphers, ":");
   while(token)

Signature · 1 line

2.1.4

Arne Schwabe

2015-09-03 06:16 UTC #1 lore raw

Am 26.08.15 um 00:29 schrieb Steffan Karger:

Quoted 9 lines

As reported by Bill Parker in trac #600, strdup() return values are not always correctly checked for failed allocations. This patch adds missing checks.
Note that in misc.c and options.c, the check is after the dirname() or basename() call, because these can deal with NULL params and we need to keep MSVC happy with its ancient no-declaration-after-statement policy.

ACK from me.

Arne

Gert Doering

2015-09-04 20:55 UTC #2 lore raw

In reply to Steffan Karger

Hi,

Quoted 7 lines · Steffan Karger

As reported by Bill Parker in trac #600, strdup() return values are not always correctly checked for failed allocations. This patch adds missing checks.
Note that in misc.c and options.c, the check is after the dirname() or basename() call, because these can deal with NULL params and we need to keep MSVC happy with its ancient no-declaration-after-statement policy.

Even though Arne has already ACKed it, I'm not merging it - I started merging it today, but the more I stare at the code, the more I'm convinced that we should just get rid of strdup() instead and/or the code is not needed.

Why...? Let me comment...

src/openvpn/cryptoapi.c static char *ms_error_text(DWORD ms_err) · line 132

Quoted 11 lines

src/openvpn/cryptoapi.c +1 -0

index b7fc11e..62f30be 100644
@@ -127,6 +127,7 @@ static char *ms_error_text(DWORD ms_err)
 		    break;
 	    }
     }
+    check_malloc_return(rv);
     return rv;
 }

This is not needed, as the code does

        rv = strdup(lpMsgBuf);
        LocalFree(lpMsgBuf);
        /* trim to the left */
        if (rv)
            for (p = rv + strlen(rv) - 1; p >= rv; p--) {
                if (isspace(*p))
                    *p = '\0';
                else
                    break;
            }
    }
    return rv;

... so if the strdup() fails, the function falls into "return NULL", which is the same return value as for "if (!lpMsgBuf)", so I assume the caller expects this - with the extra check, a failure in FormatMessage() would be flagged as memory allocation failure (because rv is set to NULL at start), which it isn't.

So, just not needed.

src/openvpn/init.c init_options_dev (struct options *options) · line 823

Quoted 12 lines

src/openvpn/init.c +1 -0

index b7c153b..0809cc7 100644
@@ -817,6 +817,7 @@ init_options_dev (struct options *options)
 {
   if (!options->dev && options->dev_node) {
     char *dev_node = strdup(options->dev_node); /* POSIX basename() implementaions may modify its arguments */
+    check_malloc_return(dev_node);
     options->dev = basename (dev_node);
   }
 }

I'd need to look into this in more detail, but I think we never modify options->dev_node afterward - so a simple strrchr() to find the "/" and point options->dev there might be sufficient... but if it actually needs to be a true copy, we should just use

char *dev_node = string_alloc(options->dev_node, NULL)

instead - string_alloc() is OpenVPN's strdup(), and "NULL" tells it "do not use gc, use calloc" (side note: someone seriously overengineered the "we really want clean memory!" bit here - allocating <n> bytes and immediately overwriting with a n-byte memcpy() should be good enough, no?) - *and* string_alloc() actually does...

      if (gc) {
...
      } else {
        ret = calloc(1, n);
        check_malloc_return(ret);
      }
      memcpy (ret, str, n);

... so the check_malloc_return() is built-in.

src/openvpn/misc.c argv_extract_cmd_name (const char *path) · line 1614

Quoted 12 lines

src/openvpn/misc.c +1 -0

index a5bad0d..b543b65 100644
@@ -1608,6 +1608,7 @@ argv_extract_cmd_name (const char *path)
     {
       char *path_cp = strdup(path); /* POSIX basename() implementaions may modify its arguments */
       const char *bn = basename (path_cp);
+      check_malloc_return(path_cp);
       if (bn)
 	{
 	  char *ret = string_alloc (bn, NULL);

This is totally weird code, using strdup() first, and then string_alloc() later, inside an "if (bn)" check on a variable that will never be NULL anyway, since basename(NULL) is documented to return "." - and it very much looks like two different persons wrote this part :-)

Dunno, this really smells like refactor the function to use strrchr() on the original path, and then use string_alloc($ptr, NULL) to make a copy of that... throw out the if(bn) block, and be done... or just change the strdup() to string_alloc() as well.

src/openvpn/options.c check_file_access(const int type, const char *file, const int mode, const char * · line 2594

Quoted 12 lines

src/openvpn/options.c +1 -0

index 2784580..dba7c9d 100644
@@ -2588,6 +2588,7 @@ check_file_access(const int type, const char *file, const int mode, const char *
     {
       char *fullpath = strdup(file);  /* POSIX dirname() implementaion may modify its arguments */
       char *dirpath = dirname(fullpath);
+      check_malloc_return(fullpath);
 
       if (platform_access (dirpath, mode|X_OK) != 0)
           errcode = errno;

string_alloc() (pity we do not have a gc around here :-) )

src/openvpn/ssl_polarssl.c tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers) · line 204

Quoted 12 lines

src/openvpn/ssl_polarssl.c +1 -0

index 3fc811e..673dbbe 100644
@@ -198,6 +198,7 @@ tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers)
   /* Parse allowed ciphers, getting IDs */
   i = 0;
   tmp_ciphers_orig = tmp_ciphers = strdup(ciphers);
+  check_malloc_return(tmp_ciphers);
 
   token = strtok (tmp_ciphers, ":");
   while(token)

I could mention string_alloc() again, but I'm sure interested readers already recognize the pattern :-))

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Steffan Karger

author

2015-10-04 14:00 UTC #3 lore raw

Quoted 27 lines · Gert Doering

src/openvpn/cryptoapi.c static char *ms_error_text(DWORD ms_err) · line 132

Quoted 11 lines

src/openvpn/cryptoapi.c +1 -0

index b7fc11e..62f30be 100644
@@ -127,6 +127,7 @@ static char *ms_error_text(DWORD ms_err)
  		    break;
  	    }
      }
+    check_malloc_return(rv);
      return rv;
  }

This is not needed, as the code does

         rv = strdup(lpMsgBuf);
         LocalFree(lpMsgBuf);
         /* trim to the left */
         if (rv)
             for (p = rv + strlen(rv) - 1; p >= rv; p--) {
                 if (isspace(*p))
                     *p = '\0';
                 else
                     break;
             }
     }
     return rv;

Quoted 7 lines

... so if the strdup() fails, the function falls into "return NULL", which is the same return value as for "if (!lpMsgBuf)", so I assume the caller expects this - with the extra check, a failure in FormatMessage() would be flagged as memory allocation failure (because rv is set to NULL at start), which it isn't.
So, just not needed.

This is what I thought at first too. But when I tried to verify that returning NULL could do no harm, I simply couldn't. The NULL is passed as a 'char *' to openssl, which stores it in some kind of error string storage. I could not trivially see that this can not do harm when actual error occur, so I decided to fix that as part of the patch.

Quoted 34 lines

src/openvpn/init.c init_options_dev (struct options *options) · line 823
Quoted 12 lines
src/openvpn/init.c +1 -0
index b7c153b..0809cc7 100644
@@ -817,6 +817,7 @@ init_options_dev (struct options *options)
  {
    if (!options->dev && options->dev_node) {
      char *dev_node = strdup(options->dev_node); /* POSIX basename() implementaions may modify its arguments */
+    check_malloc_return(dev_node);
      options->dev = basename (dev_node);
    }
  }
I'd need to look into this in more detail, but I think we never modify options->dev_node afterward - so a simple strrchr() to find the "/" and point options->dev there might be sufficient... but if it actually needs to be a true copy, we should just use
   char *dev_node = string_alloc(options->dev_node, NULL)
instead - string_alloc() is OpenVPN's strdup(), and "NULL" tells it "do not use gc, use calloc" (side note: someone seriously overengineered the "we really want clean memory!" bit here - allocating <n> bytes and immediately overwriting with a n-byte memcpy() should be good enough, no?) - *and* string_alloc() actually does...
       if (gc) {
...
       } else {
         ret = calloc(1, n);
         check_malloc_return(ret);
       }
       memcpy (ret, str, n);

Quoted 1 line

... so the check_malloc_return() is built-in.

You are absolutely right, I forgot about string_alloc(). I updated the patch to use string_alloc() instead.

So, attached two patches.

0001 replaces strdup() calls with string_alloc() to automatically check the return values.

0002 adds a return value check for the result of ms_error_text()

I ran our test suite on linux and did some smoke tests on Windows, but since this is cornercase-code, it is hard to really test.

-Steffan

From b748ddef2a779a7fa6999cb4c7bba3acf2c38f6f Mon Sep 17 00:00:00 2001
From: Steffan Karger <[email protected]...>
Date: Mon, 21 Sep 2015 22:04:19 +0200
Subject: [PATCH 2/2] Check return value of ms_error_text()

ms_error_text() may return NULL, and it is unclear (or, at least undocumented) whether the OpenSSL ERR code (and our code using the ERR code) can deal with esd->string being NULL. So, just to be sure, check that ms_error_text() succeeded before passing the result to ERR_load_strings().

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/cryptoapi.c | 1 +
 1 file changed, 1 insertion(+)

src/openvpn/cryptoapi.c +1 -0

index 1d54ee7..853c07b 100644
@@ -164,6 +164,7 @@ static void err_put_ms_error(DWORD ms_err, int func, const char *file, int line)
 	    err_map[i].ms_err = ms_err;
 	    err_map[i].err = esd->error = i + 100;
 	    esd->string = ms_error_text(ms_err);
+	    check_malloc_return(esd->string);
 	    ERR_load_strings(ERR_LIB_CRYPTOAPI, esd);
 	    ERR_PUT_error(ERR_LIB_CRYPTOAPI, func, err_map[i].err, file, line);
 	    break;

Signature · 1 line

2.1.4

From 00a71fa221f2265ad525d1b490a8fbfd009c9f23 Mon Sep 17 00:00:00 2001
From: Steffan Karger <[email protected]...>
Date: Mon, 21 Sep 2015 20:48:33 +0200
Subject: [PATCH 1/2] Replace strdup() calls for string_alloc() calls

As reported by Bill Parker in trac #600, strdup() return values are not always correctly checked for failed allocations. This patch adds missing checks by using string_alloc(), which performs the required checks.

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/buffer.h       | 2 +-
 src/openvpn/cryptoapi.c    | 4 +++-
 src/openvpn/init.c         | 2 +-
 src/openvpn/misc.c         | 2 +-
 src/openvpn/options.c      | 2 +-
 src/openvpn/ssl_polarssl.c | 2 +-
 6 files changed, 8 insertions(+), 6 deletions(-)

src/openvpn/buffer.h +1 -1

index 0dc511b..24f52aa 100644
@@ -898,7 +898,7 @@ gc_reset (struct gc_arena *a)
 }
 
 static inline void
-check_malloc_return (void *p)
+check_malloc_return (const void *p)
 {
   if (!p)
     out_of_memory ();

src/openvpn/cryptoapi.c +3 -1

index b7fc11e..1d54ee7 100644
@@ -46,6 +46,8 @@
 #include <ctype.h>
 #include <assert.h>
 
+#include "buffer.h"
+
 /* MinGW w32api 3.17 is still incomplete when it comes to CryptoAPI while
  * MinGW32-w64 defines all macros used. This is a hack around that problem.
  */

@@ -116,7 +118,7 @@ static char *ms_error_text(DWORD ms_err)
 	(LPTSTR) &lpMsgBuf, 0, NULL);
     if (lpMsgBuf) {
 	char *p;
-	rv = strdup(lpMsgBuf);
+	rv = string_alloc(lpMsgBuf, NULL);
 	LocalFree(lpMsgBuf);
 	/* trim to the left */
 	if (rv)

src/openvpn/init.c +1 -1

index f568d87..3decd23 100644
@@ -822,7 +822,7 @@ void
 init_options_dev (struct options *options)
 {
   if (!options->dev && options->dev_node) {
-    char *dev_node = strdup(options->dev_node); /* POSIX basename() implementaions may modify its arguments */
+    char *dev_node = string_alloc(options->dev_node, NULL); /* POSIX basename() implementaions may modify its arguments */
     options->dev = basename (dev_node);
   }
 }

src/openvpn/misc.c +1 -1

index 895e9fa..fd1930a 100644
@@ -1615,7 +1615,7 @@ argv_extract_cmd_name (const char *path)
 {
   if (path)
     {
-      char *path_cp = strdup(path); /* POSIX basename() implementaions may modify its arguments */
+      char *path_cp = string_alloc(path, NULL); /* POSIX basename() implementaions may modify its arguments */
       const char *bn = basename (path_cp);
       if (bn)
 	{

src/openvpn/options.c +1 -1

index 5ace1f3..de4fa38 100644
@@ -2578,7 +2578,7 @@ check_file_access(const int type, const char *file, const int mode, const char *
   /* Is the directory path leading to the given file accessible? */
   if (type & CHKACC_DIRPATH)
     {
-      char *fullpath = strdup(file);  /* POSIX dirname() implementaion may modify its arguments */
+      char *fullpath = string_alloc (file, NULL);  /* POSIX dirname() implementaion may modify its arguments */
       char *dirpath = dirname(fullpath);
 
       if (platform_access (dirpath, mode|X_OK) != 0)

src/openvpn/ssl_polarssl.c +1 -1

index dd0fab0..11c9ffb 100644
@@ -197,7 +197,7 @@ tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers)
 
   /* Parse allowed ciphers, getting IDs */
   i = 0;
-  tmp_ciphers_orig = tmp_ciphers = strdup(ciphers);
+  tmp_ciphers_orig = tmp_ciphers = string_alloc (ciphers, NULL);
 
   token = strtok (tmp_ciphers, ":");
   while(token)

Signature · 1 line

2.1.4

Gert Doering

2015-10-04 20:33 UTC #4 lore raw

[Openvpn-devel] [PATCH applied] Re: Replace strdup() calls for string_alloc() calls

ACK.

Your patches have been applied to the master and release/2.3 branch.

commit ddc7692d245017c71adc40ad5cc195617e39fce0 (master) commit 6d4920e9d36675ff515da17c1eb301d22d07b489 (release/2.3)

Author: Steffan Karger
Date:   Mon Sep 21 20:48:33 2015 +0200

     Replace strdup() calls for string_alloc() calls

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/10176
     Signed-off-by: Gert Doering <[email protected]...>

commit 5584b738a332d0abc740d9303c275764c2ca13f1 (master) commit ad71fe5f79ffbd4c86afa79d4559b08617b41dfb (release/2.3)

Author: Steffan Karger
Date:   Mon Sep 21 22:04:19 2015 +0200

     Check return value of ms_error_text()

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/10176
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering