[Openvpn-devel,v2] interactive.c: fix usage of potentially uninitialized variable

  From: Lev Stipakov <lev@openvpn.net>

In function netsh_dns_cmd() it is possible to jump on a label and
call free() on uninitialized pointer. Move pointer initialization
above jump.

To fix a few warnings which are treated as errors with SDL enabled,
initialize pointers with NULL.

Signed-off-by: Lev Stipakov <lev@openvpn.net>
---
v2:
 - remove NULL check before free() call
 - remove unneeded NULL initializations in function body

 src/openvpnserv/interactive.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
  

Hi,

Sorry I missed this patch cleaning up my mistake..

Gert has already reviewed and asked for this v2 so this may be redundant,
but fwiw:

On Mon, Oct 8, 2018 at 2:15 PM Lev Stipakov <lstipakov@gmail.com> wrote:

> From: Lev Stipakov <lev@openvpn.net>
>
> In function netsh_dns_cmd() it is possible to jump on a label and
> call free() on uninitialized pointer. Move pointer initialization
> above jump.
>
> To fix a few warnings which are treated as errors with SDL enabled,
> initialize pointers with NULL.
>
> Signed-off-by: Lev Stipakov <lev@openvpn.net>
> ---
> v2:
>  - remove NULL check before free() call
>  - remove unneeded NULL initializations in function body
>
>  src/openvpnserv/interactive.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
> index 8f92c24..4a571f5 100644
> --- a/src/openvpnserv/interactive.c
> +++ b/src/openvpnserv/interactive.c
> @@ -1015,6 +1015,7 @@ netsh_dns_cmd(const wchar_t *action, const wchar_t
> *proto, const wchar_t *if_nam
>      DWORD err = 0;
>      int timeout = 30000; /* in msec */
>      wchar_t argv0[MAX_PATH];
> +    wchar_t *cmdline = NULL;
>

Indeed this has to move up or the early jump to label out should be
replaced by "return err".

Is there a way to get gcc warn (or error) when jumping over unitialized
variables
that get referenced after the jump --  -Wjump-misses-init is noisy and
does not catch all.


>      if (!addr)
>      {
> @@ -1039,7 +1040,7 @@ netsh_dns_cmd(const wchar_t *action, const wchar_t
> *proto, const wchar_t *if_nam
>
>      /* max cmdline length in wchars -- include room for worst case and
> some */
>      size_t ncmdline = wcslen(fmt) + wcslen(if_name) + wcslen(addr) + 32 +
> 1;
> -    wchar_t *cmdline = malloc(ncmdline*sizeof(wchar_t));
> +    cmdline = malloc(ncmdline*sizeof(wchar_t));
>      if (!cmdline)
>      {
>          err = ERROR_OUTOFMEMORY;
> @@ -1350,7 +1351,7 @@ RunOpenvpn(LPVOID p)
>  {
>      HANDLE pipe = p;
>      HANDLE ovpn_pipe, svc_pipe;
> -    PTOKEN_USER svc_user, ovpn_user;
> +    PTOKEN_USER svc_user = NULL, ovpn_user = NULL;
>

Good catch -- these could get passed to free on errors.


>      HANDLE svc_token = NULL, imp_token = NULL, pri_token = NULL;
>      HANDLE stdin_read = NULL, stdin_write = NULL;
>      HANDLE stdout_write = NULL;
> @@ -1403,7 +1404,6 @@ RunOpenvpn(LPVOID p)
>          goto out;
>      }
>      len = 0;
> -    svc_user = NULL;
>      while (!GetTokenInformation(svc_token, TokenUser, svc_user, len,
> &len))
>      {
>          if (GetLastError() != ERROR_INSUFFICIENT_BUFFER)
> @@ -1436,7 +1436,6 @@ RunOpenvpn(LPVOID p)
>          goto out;
>      }
>      len = 0;
> -    ovpn_user = NULL;
>      while (!GetTokenInformation(imp_token, TokenUser, ovpn_user, len,
> &len))
>      {
>          if (GetLastError() != ERROR_INSUFFICIENT_BUFFER)
>

Thanks for the cleanup.

Acked-by: selva.nair@gmail.com
<div dir="ltr"><div><div>Hi,<br><br></div>Sorry I missed this patch cleaning up my mistake..<br><br>Gert has already reviewed and asked for this v2 so this may be redundant,<br></div>but fwiw:<br><br><div><div class="gmail_quote"><div dir="ltr">On Mon, Oct 8, 2018 at 2:15 PM Lev Stipakov &lt;<a href="mailto:lstipakov@gmail.com" target="_blank">lstipakov@gmail.com</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">From: Lev Stipakov &lt;<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>&gt;<br>
<br>
In function netsh_dns_cmd() it is possible to jump on a label and<br>
call free() on uninitialized pointer. Move pointer initialization<br>
above jump.<br>
<br>
To fix a few warnings which are treated as errors with SDL enabled,<br>
initialize pointers with NULL.<br>
<br>
Signed-off-by: Lev Stipakov &lt;<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>&gt;<br>
---<br>
v2:<br>
 - remove NULL check before free() call<br>
 - remove unneeded NULL initializations in function body<br>
<br>
 src/openvpnserv/interactive.c | 7 +++----<br>
 1 file changed, 3 insertions(+), 4 deletions(-)<br>
<br>
diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c<br>
index 8f92c24..4a571f5 100644<br>
--- a/src/openvpnserv/interactive.c<br>
+++ b/src/openvpnserv/interactive.c<br>
@@ -1015,6 +1015,7 @@ netsh_dns_cmd(const wchar_t *action, const wchar_t *proto, const wchar_t *if_nam<br>
     DWORD err = 0;<br>
     int timeout = 30000; /* in msec */<br>
     wchar_t argv0[MAX_PATH];<br>
+    wchar_t *cmdline = NULL;<br></blockquote><div><br></div><div>Indeed this has to move up or the early jump to label out should be <br>replaced by &quot;return err&quot;.<br><br>Is there a way to get gcc warn (or error) when jumping over unitialized variables<br></div><div>that get referenced after the jump --  -Wjump-misses-init is noisy and <br></div><div>does not catch all.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
     if (!addr)<br>
     {<br>
@@ -1039,7 +1040,7 @@ netsh_dns_cmd(const wchar_t *action, const wchar_t *proto, const wchar_t *if_nam<br>
<br>
     /* max cmdline length in wchars -- include room for worst case and some */<br>
     size_t ncmdline = wcslen(fmt) + wcslen(if_name) + wcslen(addr) + 32 + 1;<br>
-    wchar_t *cmdline = malloc(ncmdline*sizeof(wchar_t));<br>
+    cmdline = malloc(ncmdline*sizeof(wchar_t));<br>
     if (!cmdline)<br>
     {<br>
         err = ERROR_OUTOFMEMORY;<br>
@@ -1350,7 +1351,7 @@ RunOpenvpn(LPVOID p)<br>
 {<br>
     HANDLE pipe = p;<br>
     HANDLE ovpn_pipe, svc_pipe;<br>
-    PTOKEN_USER svc_user, ovpn_user;<br>
+    PTOKEN_USER svc_user = NULL, ovpn_user = NULL;<br></blockquote><div><br></div><div>Good catch -- these could get passed to free on errors.<br>   <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
     HANDLE svc_token = NULL, imp_token = NULL, pri_token = NULL;<br>
     HANDLE stdin_read = NULL, stdin_write = NULL;<br>
     HANDLE stdout_write = NULL;<br>
@@ -1403,7 +1404,6 @@ RunOpenvpn(LPVOID p)<br>
         goto out;<br>
     }<br>
     len = 0;<br>
-    svc_user = NULL;<br>
     while (!GetTokenInformation(svc_token, TokenUser, svc_user, len, &amp;len))<br>
     {<br>
         if (GetLastError() != ERROR_INSUFFICIENT_BUFFER)<br>
@@ -1436,7 +1436,6 @@ RunOpenvpn(LPVOID p)<br>
         goto out;<br>
     }<br>
     len = 0;<br>
-    ovpn_user = NULL;<br>
     while (!GetTokenInformation(imp_token, TokenUser, ovpn_user, len, &amp;len))<br>
     {<br>
         if (GetLastError() != ERROR_INSUFFICIENT_BUFFER)<br></blockquote><div><br></div><div>Thanks for the cleanup.<br><br></div><div>Acked-by: <a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a></div></div></div></div>
  

Your patch has been applied to the master and release/2.4 branch (bugfix).

Thanks, Selva, for the extra pair of eyes.  I was fine with the v2, but
had hoped to see an extra review from you in case I overlooked something
("you know this code best").

Test built on ubuntu 16.04.

commit d1f0e2cf83c378b4064f316a2127c7a3d7c6ca21 (master)
commit 773c086367c8dd94d71a10bd2b32f09b4a0840ae (release/2.4)
Author: Lev Stipakov
Date:   Mon Oct 8 21:12:58 2018 +0300

     interactive.c: fix usage of potentially uninitialized variable

     Signed-off-by: Lev Stipakov <lev@openvpn.net>
     Acked-by: Selva Nair <selva.nair@gmail.com>
     Message-Id: <1539022378-24485-1-git-send-email-lstipakov@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg17663.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering