interactive.c: fix usage of potentially uninitialized variable

PATCH Superseded Lev Stipakov 4 participants 7 messages Oct 8, 2018 – Oct 10, 2018

lore

Share & tools

lore

Lev Stipakov

author

2018-10-08 16:35 UTC #0 lore raw

PATCH 1/1 interactive.c: fix usage of potentially uninitialized variable

From: Lev Stipakov <[email protected]...>

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

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

Signed-off-by Lev Stipakov <[email protected]...>

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

src/openvpnserv/interactive.c +7 -3

index 8f92c24..369980b 100644
@@ -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;
 
     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;

@@ -1055,7 +1056,10 @@ netsh_dns_cmd(const wchar_t *action, const wchar_t *proto, const wchar_t *if_nam
     err = ExecCommand(argv0, cmdline, timeout);
 
 out:
-    free(cmdline);
+    if (cmdline)
+    {
+        free(cmdline);
+    }
     return err;
 }

@@ -1350,7 +1354,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;
     HANDLE svc_token = NULL, imp_token = NULL, pri_token = NULL;
     HANDLE stdin_read = NULL, stdin_write = NULL;
     HANDLE stdout_write = NULL;

Signature · 1 line

2.7.4

Gert Doering

2018-10-08 17:31 UTC #1 lore raw

Hi,

src/openvpnserv/interactive.c netsh_dns_cmd(const wchar_t *action, const wchar_t *proto, const wchar_t *if_nam · line 1062

Quoted 6 lines · Lev Stipakov

out:

src/openvpnserv/interactive.c +4 -1

-    free(cmdline);
+    if (cmdline)
+    {
+        free(cmdline);
+    }

Do we have something in our styleguides related to these constructs?

Since free(NULL) is well-defined and valid(!), the if() check is not needed at all and just adding extra lines on screen.

I'm sure we had this discussion before, but couldn't find something where we wrote down how and why we do things. We do have parts of the code that *does* "if(ptr) { free(ptr); }" checks, and others that don't... for example, AsyncPipeOp() on failure to malloc() "handles".

(Of course cmdline needs to be initialized with NULL - not argueing *that* part)

gert

Signature · 6 lines

"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             [email protected]...

Antonio Quartulli

2018-10-08 17:42 UTC #2 lore raw

Hi,

Quoted 12 lines · Gert Doering

Hi,
src/openvpnserv/interactive.c netsh_dns_cmd(const wchar_t *action, const wchar_t *proto, const wchar_t *if_nam · line 1062
Quoted 6 lines · Lev Stipakov
out:
src/openvpnserv/interactive.c +4 -1
-    free(cmdline);
+    if (cmdline)
+    {
+        free(cmdline);
+    }
Do we have something in our styleguides related to these constructs?

Quoted 8 lines

Since free(NULL) is well-defined and valid(!), the if() check is not needed at all and just adding extra lines on screen.
I'm sure we had this discussion before, but couldn't find something where we wrote down how and why we do things. We do have parts of the code that *does* "if(ptr) { free(ptr); }" checks, and others that don't... for example, AsyncPipeOp() on failure to malloc() "handles".

I don't think we have any guideline, but I agree we should just remove the surrounding check, because, as you said, it is well defined behaviour.

Maybe somebody could make this consistent across the codebase.

Regards,

Signature · 1 line

Antonio Quartulli

Gert Doering

2018-10-08 17:52 UTC #3 lore raw

In reply to Lev Stipakov

Hi,

src/openvpnserv/interactive.c RunOpenvpn(LPVOID p) · line 1360

Quoted 9 lines · Lev Stipakov

src/openvpnserv/interactive.c +1 -1

@@ -1350,7 +1354,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;
     HANDLE svc_token = NULL, imp_token = NULL, pri_token = NULL;
     HANDLE stdin_read = NULL, stdin_write = NULL;
     HANDLE stdout_write = NULL;

Oh, another nag for v2 :-) - if you initialize these to NULL here at the top, the lines

    svc_user = NULL;
...
    ovpn_user = NULL;

further down could go...

gert

Signature · 6 lines

"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             [email protected]...

Lev Stipakov

author

2018-10-08 18:12 UTC #4 lore raw

PATCH v2 1/1 interactive.c: fix usage of potentially uninitialized variable

From: Lev Stipakov <[email protected]...>

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 <[email protected]...>

---
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(-)

src/openvpnserv/interactive.c +3 -4

index 8f92c24..4a571f5 100644
@@ -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;
 
     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;
     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)

Signature · 1 line

2.7.4

Selva Nair

Acked-by on v2 1/1

2018-10-10 17:25 UTC #5 lore raw

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:

Quoted 23 lines · Lev Stipakov

From: Lev Stipakov <[email protected]...>
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 <[email protected]...>
---
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(-)
src/openvpnserv/interactive.c +0 -0
index 8f92c24..4a571f5 100644

src/openvpnserv/interactive.c netsh_dns_cmd(const wchar_t *action, const wchar_t · line 1020

Quoted 6 lines

src/openvpnserv/interactive.c +1 -0

@@ -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.

Quoted 15 lines

src/openvpnserv/interactive.c +0 -0

     if (!addr)

src/openvpnserv/interactive.c +0 -0

@@ -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;

src/openvpnserv/interactive.c +1 -1

-    wchar_t *cmdline = malloc(ncmdline*sizeof(wchar_t));
+    cmdline = malloc(ncmdline*sizeof(wchar_t));
     if (!cmdline)
     {
         err = ERROR_OUTOFMEMORY;

src/openvpnserv/interactive.c RunOpenvpn(LPVOID p) · line 1355

Quoted 6 lines

src/openvpnserv/interactive.c +1 -1

@@ -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.

Quoted 20 lines

src/openvpnserv/interactive.c +0 -0

     HANDLE svc_token = NULL, imp_token = NULL, pri_token = NULL;
     HANDLE stdin_read = NULL, stdin_write = NULL;

     HANDLE stdout_write = NULL;

src/openvpnserv/interactive.c +0 -1

@@ -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)

src/openvpnserv/interactive.c +0 -1

@@ -1436,7 +1436,6 @@ RunOpenvpn(LPVOID p)
         goto out;
     }
     len = 0;
-    ovpn_user = NULL;
     while (!GetTokenInformation(imp_token, TokenUser, ovpn_user, len,
 &len))
     {

Quoted 2 lines

         if (GetLastError() != ERROR_INSUFFICIENT_BUFFER)

Thanks for the cleanup.

Acked-by [email protected]...

Gert Doering

2018-10-10 18:19 UTC #6 lore raw

In reply to Lev Stipakov

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 <[email protected]...>
     Acked-by: Selva Nair <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg17663.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering