Skip to contentSkip to conversation

Properly handle possible realloc failure

PATCH In review Selva Nair 2 participants 12 messages May 14, 2016 – May 22, 2016

lore
Share & tools
lore

Selva Nair

author
#0 lore raw

PATCH 1/1 Properly handle possible realloc failure

Signed-off-by Selva Nair <[email protected]...>
---
 src/openvpnserv/interactive.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
src/openvpnserv/interactive.c +7 -3
index d83ea65..5ff80fa 100644
@@ -1326,15 +1326,19 @@ UpdateWaitHandles (LPHANDLE *handles_ptr, LPDWORD count,
       if (pos == size)
         {
           size += 10;
-          handles = realloc (handles, size * sizeof (HANDLE));
-          if (handles == NULL)
+          LPHANDLE tmp = realloc (handles, size * sizeof (HANDLE));
+          if (tmp == NULL)
             return ERROR_OUTOFMEMORY;
+          else
+            {
+              handles = tmp;
+              *handles_ptr = handles;
+            }
         }
       handles[pos++] = threads->data;
       threads = threads->next;
     }
 
-  *handles_ptr = handles;
   *count = pos;
   return NO_ERROR;
 }
Signature · 1 line
1.7.10.4
author
#1 lore raw
Quoted 4 lines · Selva Nair
---
 src/openvpnserv/interactive.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Please ignore this patch. It is broken -- patch made prematurely before a fixup was committed,

My apologies for the noise..

Selva

author
#2 lore raw
Quoted 11 lines · Selva Nair
Quoted 4 lines · Selva Nair
---
 src/openvpnserv/interactive.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Please ignore this patch. It is broken -- patch made prematurely before a fixup was committed,

While redoing this patch I tested what actually happens on malloc or realloc failure and found a real bug. The patch in the next email addresses that.

Selva

author
#3 lore raw

PATCH 1/1 Fix handling of out of memory error in interactive service

Currently realloc failure in UpdateWaitHandles() is handled by triggering exit_event and waiting for all active worker threads to terminate. However, at this point the wait handles array will contain an invalid value (handle of the latest thread that is terminated), causing a cycle of WAIT_FAILED <-> continue and trashing of the eventlog.

Fix:
- Update the wait handles again after removing the last thread:
this should not fail as no extra memory is needed. Do not set the exit event; existing connections are not terminated.

- In case of WAIT_FAILED, break out of the while loop and exit instead of continue. This usually happens when one or more handles are invalid, which is hard to recover from.

Other changes:
- Use minimal initial allocation size so that the realloc code path gets exercised (2 or more connections will cause realloc).
- Use a temp variable to check the return value of realloc().
- Initialize handles array pointer to NULL.

Tested using a dummy realloc that returns NULL.

Signed-off-by Selva Nair <[email protected]...>
---
 src/openvpnserv/interactive.c |   35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)
src/openvpnserv/interactive.c +24 -11
index df30ad7..ccbf75d 100644
@@ -1453,13 +1453,14 @@ static DWORD
 UpdateWaitHandles (LPHANDLE *handles_ptr, LPDWORD count,
                    HANDLE io_event, HANDLE exit_event, list_item_t *threads)
 {
-  static DWORD size = 10;
+  static DWORD size = 2;
   static LPHANDLE handles = NULL;
   DWORD pos = 0;
 
   if (handles == NULL)
     {
       handles = malloc (size * sizeof (HANDLE));
+      *handles_ptr = handles;
       if (handles == NULL)
         return ERROR_OUTOFMEMORY;
     }

@@ -1474,15 +1475,20 @@ UpdateWaitHandles (LPHANDLE *handles_ptr, LPDWORD count,
       if (pos == size)
         {
           size += 10;
-          handles = realloc (handles, size * sizeof (HANDLE));
-          if (handles == NULL)
-            return ERROR_OUTOFMEMORY;
+          LPHANDLE tmp = realloc (handles, size * sizeof (HANDLE));
+          if (tmp == NULL)
+            {
+              size -= 10;
+              *count = pos;
+              return ERROR_OUTOFMEMORY;
+            }
+          handles = tmp;
+          *handles_ptr = handles;
         }
       handles[pos++] = threads->data;
       threads = threads->next;
     }
 
-  *handles_ptr = handles;
   *count = pos;
   return NO_ERROR;
 }

@@ -1502,8 +1508,9 @@ ServiceStartInteractive (DWORD dwArgc, LPTSTR *lpszArgv)
   OVERLAPPED overlapped;
   DWORD error = NO_ERROR;
   list_item_t *threads = NULL;
-  PHANDLE handles;
+  PHANDLE handles = NULL;
   DWORD handle_count;
+  BOOL CmpHandle (LPVOID item, LPVOID hnd) { return item == hnd; }
 
   service = RegisterServiceCtrlHandlerEx (interactive_service.name, ServiceCtrlInteractive, &status);
   if (!service)

@@ -1566,14 +1573,18 @@ ServiceStartInteractive (DWORD dwArgc, LPTSTR *lpszArgv)
           HANDLE thread = CreateThread (NULL, 0, RunOpenvpn, pipe, CREATE_SUSPENDED, NULL);
           if (thread)
             {
-              error = AddListItem (&threads, thread) ||
-                UpdateWaitHandles (&handles, &handle_count, io_event, exit_event, threads);
+              error = AddListItem (&threads, thread);
+              if (!error)
+                error = UpdateWaitHandles (&handles, &handle_count, io_event, exit_event, threads);
               if (error)
                 {
+                  ReturnError (pipe, error, L"Insufficient reources to service new clients", 1, &exit_event);
+                  /* Update wait handles again after removing the last worker thread */
+                  RemoveListItem (&threads, CmpHandle, thread);
+                  UpdateWaitHandles (&handles, &handle_count, io_event, exit_event, threads);
                   TerminateThread (thread, 1);
                   CloseHandleEx (&thread);
                   CloseHandleEx (&pipe);
-                  SetEvent (exit_event);
                 }
               else
                 ResumeThread (thread);

@@ -1590,7 +1601,10 @@ ServiceStartInteractive (DWORD dwArgc, LPTSTR *lpszArgv)
           if (error == WAIT_FAILED)
             {
               MsgToEventLog (M_SYSERR, TEXT("WaitForMultipleObjects failed"));
-              continue;
+              SetEvent (exit_event);
+              /* Give some time for worker threads to exit and then terminate */
+              Sleep (1000);
+              break;
             }
           if (!threads)
             {

@@ -1602,7 +1616,6 @@ ServiceStartInteractive (DWORD dwArgc, LPTSTR *lpszArgv)
             }
 
           /* Worker thread ended */
-          BOOL CmpHandle (LPVOID item, LPVOID hnd) { return item == hnd; }
           HANDLE thread = RemoveListItem (&threads, CmpHandle, handles[error]);
           UpdateWaitHandles (&handles, &handle_count, io_event, exit_event, threads);
           CloseHandleEx (&thread);
Signature · 1 line
1.7.10.4
#4 lore raw

Re: [Openvpn-devel] [PATCH] Fix handling of out of memory error in interactive service

Hi,

thanks for digging into this. Overall, the patch makes sense, I'm just wondering about this:

Quoted 3 lines · Selva Nair

Other changes:
- Use minimal initial allocation size so that the realloc code path gets exercised (2 or more connections will cause realloc).

I can see that this is useful for testing, but do we want this in production?

It won't make a difference for most usages ("only one connection active"), but then, the extra memory use for 10 is minimal, and realloc() *can* fail, so just keeping at 10 won't need the realloc() path generally...

Dunno, just thinking loud.

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]...
author
#5 lore raw

Re: [Openvpn-devel] [PATCH] Fix handling of out of memory error in interactive service

Hi,

Thanks for the comments.

Quoted 8 lines · Gert Doering

thanks for digging into this. Overall, the patch makes sense, I'm just wondering about this:

Quoted 3 lines · Selva Nair

Other changes:
- Use minimal initial allocation size so that the realloc code path gets exercised (2 or more connections will cause realloc).

Quoted 8 lines

I can see that this is useful for testing, but do we want this in production?

It won't make a difference for most usages ("only one connection active"), but then, the extra memory use for 10 is minimal, and realloc() *can* fail, so just keeping at 10 won't need the realloc() path generally...

Of course, the extra memory usage is not a concern. As a general practice I like malloc/realloc for growing arrays to start from the smallest possible value to expose bugs in the logic during normal use[*].

That said, I'm ok with reverting it back to 10 or even a somewhat larger value -- I understand the "in case the fix is buggy, be sure it doesn't break anything" kind of feeling.

Selva

[*] See similar advice in "The Practice of Programming", section on growing arrays..

#6 lore raw

Re: [Openvpn-devel] [PATCH] Fix handling of out of memory error in interactive service

In reply to Selva Nair

Hi,

src/openvpnserv/interactive.c UpdateWaitHandles (LPHANDLE *handles_ptr, LPDWORD count, · line 1479

Quoted 7 lines · Selva Nair
         {
           size += 10;
src/openvpnserv/interactive.c +2 -3
-          handles = realloc (handles, size * sizeof (HANDLE));
-          if (handles == NULL)
-            return ERROR_OUTOFMEMORY;
+          LPHANDLE tmp = realloc (handles, size * sizeof (HANDLE));
+          if (tmp == NULL)

... oh, and this might actually fail MSVC compilation, due to "variable declared after other code parts" (size += 10)...

src/openvpnserv/interactive.c ServiceStartInteractive (DWORD dwArgc, LPTSTR *lpszArgv) · line 1576

Quoted 10 lines
               if (error)
                 {
src/openvpnserv/interactive.c +4 -1
+                  ReturnError (pipe, error, L"Insufficient reources to service new clients", 1, &exit_event);
+                  /* Update wait handles again after removing the last worker thread */
+                  RemoveListItem (&threads, CmpHandle, thread);
+                  UpdateWaitHandles (&handles, &handle_count, io_event, exit_event, threads);
                   TerminateThread (thread, 1);
                   CloseHandleEx (&thread);
                   CloseHandleEx (&pipe);
-                  SetEvent (exit_event);

So we do not exit anymore when this fails, except if WAIT_FAILED?

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]...
author
#7 lore raw

Re: [Openvpn-devel] [PATCH] Fix handling of out of memory error in interactive service

copy to the list...

Quoted 11 lines · Gert Doering

Hi,

src/openvpnserv/interactive.c UpdateWaitHandles (LPHANDLE *handles_ptr, LPDWORD count, · line 1479

Quoted 7 lines · Selva Nair
         {
           size += 10;
src/openvpnserv/interactive.c +2 -3
-          handles = realloc (handles, size * sizeof (HANDLE));
-          if (handles == NULL)
-            return ERROR_OUTOFMEMORY;
+          LPHANDLE tmp = realloc (handles, size * sizeof (HANDLE));
+          if (tmp == NULL)
Quoted 3 lines

... oh, and this might actually fail MSVC compilation, due to "variable declared after other code parts" (size += 10)...

Isn't it time to deprecate support for visual studio older than 2013? MSVC in VS-2015 supports C99 constructs and VS2013 may also do.

If not, I'll submit a version 2.

Quoted 9 lines
Quoted 4 lines
               if (error)
                 {

+ ReturnError (pipe, error, L"Insufficient reources to

service new clients", 1, &exit_event);

Quoted 1 line

+ /* Update wait handles again after removing the last

worker thread */

Review

Quoted 2 lines
(quoted diff) +2 -0
+                  RemoveListItem (&threads, CmpHandle, thread);
+                  UpdateWaitHandles (&handles, &handle_count, io_event,
Quoted 8 lines

exit_event, threads);

Quoted 4 lines
                   TerminateThread (thread, 1);
                   CloseHandleEx (&thread);
                   CloseHandleEx (&pipe);

- SetEvent (exit_event);

So we do not exit anymore when this fails, except if WAIT_FAILED?

That is correct. I tested it and works nicely -- i.e., on realloc error, all previous connections continue while new connections get an error message.

Selva

#8 lore raw

Re: [Openvpn-devel] [PATCH] Fix handling of out of memory error in interactive service

Hi,

Quoted 9 lines · Selva Nair
Quoted 9 lines · Selva Nair
Quoted 7 lines
         {
           size += 10;
src/openvpnserv/interactive.c +2 -3
-          handles = realloc (handles, size * sizeof (HANDLE));
-          if (handles == NULL)
-            return ERROR_OUTOFMEMORY;
+          LPHANDLE tmp = realloc (handles, size * sizeof (HANDLE));
+          if (tmp == NULL)
Quoted 7 lines
Quoted 2 lines

... oh, and this might actually fail MSVC compilation, due to "variable declared after other code parts" (size += 10)...

Isn't it time to deprecate support for visual studio older than 2013? MSVC in VS-2015 supports C99 constructs and VS2013 may also do.

If not, I'll submit a version 2.

Dunno... the only time I compiled on windows, I was using mingw+msys... but since I get yelled at if we break MSVC compatibility, I started to look out for such constructs.

Lev, what are you compiling with?

[..]

Quoted 5 lines
Quoted 1 line

So we do not exit anymore when this fails, except if WAIT_FAILED?

That is correct. I tested it and works nicely -- i.e., on realloc error, all previous connections continue while new connections get an error message.

This is very nice indeed!

(Not that a realloc() failure for +40 bytes would ever happen on its own - if memory is that tight, other stuff will explode as well :-) - but I really really like such defensive code that just works with what it has, instead of "oh, panic, just crash")

Mmmh, anyway, I have two options now :-) - ask you for a v2 that is MSVC 2010 compatible, and has the initial value reverted to "10", or just apply...

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]...
author
#9 lore raw

Re: [Openvpn-devel] [PATCH] Fix handling of out of memory error in interactive service

Quoted 4 lines · Gert Doering

Mmmh, anyway, I have two options now :-) - ask you for a v2 that is MSVC 2010 compatible, and has the initial value reverted to "10", or just apply...

Let's make life easy -- will do a v2 with both those changes:-) and just hope if/when we touch that realloc loop in future we'll test it by exercising it... I normally run 2 connections on my laptop (sometimes 3), never 10.

As a general rule, which MSVC version to target is still useful to know as there are a few places in the code requiring ugly scoping brackets as seen in some pending patches for MSVC compliance.

Selva

author
#10 lore raw

PATCH v2 1/1 Fix handling of out of memory error in interactive service

In reply to Selva Nair

Currently realloc failure in UpdateWaitHandles() is handled by triggering exit_event and waiting for all active worker threads to terminate. However, at this point the wait handles array will contain an invalid value (handle of the latest thread that is terminated), causing a cycle of WAIT_FAILED <-> continue and trashing of the eventlog.

Fix:
- Update the wait handles again after removing the last thread:
this should not fail as no extra memory is needed. Do not set the exit event; existing connections are not terminated.

- In case of WAIT_FAILED, break out of the while loop and exit instead of continue. This usually happens when one or more handles are invalid, which is hard to recover from.

Other changes:
- Use minimal initial allocation size so that the realloc code path gets exercised (2 or more connections will cause realloc).
- Use a temp variable to check the return value of realloc().
- Initialize handles array pointer to NULL.

v2 changes:
- Increased initial allocation to 10 (warn: now 10 or more connections needed to exercise the realloc code path).
- Moved up the declaration of "LPHANDLE tmp" to please stone-age MSVC.

Tested using a dummy realloc that returns NULL.

Signed-off-by Selva Nair <[email protected]...>
---
 src/openvpnserv/interactive.c |   34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)
src/openvpnserv/interactive.c +24 -10
index df30ad7..2453f17 100644
@@ -1460,6 +1460,7 @@ UpdateWaitHandles (LPHANDLE *handles_ptr, LPDWORD count,
   if (handles == NULL)
     {
       handles = malloc (size * sizeof (HANDLE));
+      *handles_ptr = handles;
       if (handles == NULL)
         return ERROR_OUTOFMEMORY;
     }

@@ -1473,16 +1474,22 @@ UpdateWaitHandles (LPHANDLE *handles_ptr, LPDWORD count,
     {
       if (pos == size)
         {
+          LPHANDLE tmp;
           size += 10;
-          handles = realloc (handles, size * sizeof (HANDLE));
-          if (handles == NULL)
-            return ERROR_OUTOFMEMORY;
+          tmp = realloc (handles, size * sizeof (HANDLE));
+          if (tmp == NULL)
+            {
+              size -= 10;
+              *count = pos;
+              return ERROR_OUTOFMEMORY;
+            }
+          handles = tmp;
+          *handles_ptr = handles;
         }
       handles[pos++] = threads->data;
       threads = threads->next;
     }
 
-  *handles_ptr = handles;
   *count = pos;
   return NO_ERROR;
 }

@@ -1502,8 +1509,9 @@ ServiceStartInteractive (DWORD dwArgc, LPTSTR *lpszArgv)
   OVERLAPPED overlapped;
   DWORD error = NO_ERROR;
   list_item_t *threads = NULL;
-  PHANDLE handles;
+  PHANDLE handles = NULL;
   DWORD handle_count;
+  BOOL CmpHandle (LPVOID item, LPVOID hnd) { return item == hnd; }
 
   service = RegisterServiceCtrlHandlerEx (interactive_service.name, ServiceCtrlInteractive, &status);
   if (!service)

@@ -1566,14 +1574,18 @@ ServiceStartInteractive (DWORD dwArgc, LPTSTR *lpszArgv)
           HANDLE thread = CreateThread (NULL, 0, RunOpenvpn, pipe, CREATE_SUSPENDED, NULL);
           if (thread)
             {
-              error = AddListItem (&threads, thread) ||
-                UpdateWaitHandles (&handles, &handle_count, io_event, exit_event, threads);
+              error = AddListItem (&threads, thread);
+              if (!error)
+                error = UpdateWaitHandles (&handles, &handle_count, io_event, exit_event, threads);
               if (error)
                 {
+                  ReturnError (pipe, error, L"Insufficient resources to service new clients", 1, &exit_event);
+                  /* Update wait handles again after removing the last worker thread */
+                  RemoveListItem (&threads, CmpHandle, thread);
+                  UpdateWaitHandles (&handles, &handle_count, io_event, exit_event, threads);
                   TerminateThread (thread, 1);
                   CloseHandleEx (&thread);
                   CloseHandleEx (&pipe);
-                  SetEvent (exit_event);
                 }
               else
                 ResumeThread (thread);

@@ -1590,7 +1602,10 @@ ServiceStartInteractive (DWORD dwArgc, LPTSTR *lpszArgv)
           if (error == WAIT_FAILED)
             {
               MsgToEventLog (M_SYSERR, TEXT("WaitForMultipleObjects failed"));
-              continue;
+              SetEvent (exit_event);
+              /* Give some time for worker threads to exit and then terminate */
+              Sleep (1000);
+              break;
             }
           if (!threads)
             {

@@ -1602,7 +1617,6 @@ ServiceStartInteractive (DWORD dwArgc, LPTSTR *lpszArgv)
             }
 
           /* Worker thread ended */
-          BOOL CmpHandle (LPVOID item, LPVOID hnd) { return item == hnd; }
           HANDLE thread = RemoveListItem (&threads, CmpHandle, handles[error]);
           UpdateWaitHandles (&handles, &handle_count, io_event, exit_event, threads);
           CloseHandleEx (&thread);
Signature · 1 line
1.7.10.4
#11 lore raw

[Openvpn-devel] [PATCH applied] Re: Fix handling of out of memory error in interactive service

ACK, thanks.

Your patch has been applied to the master branch.

commit 600dd9a16fc61ff6e595f500fba5daf14248b739

Author: Selva Nair
Date:   Sun May 22 14:39:32 2016 -0400
     Fix handling of out of memory error in interactive service

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

--
kind regards,

Gert Doering