diff mbox series

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

Message ID 1539016554-23503-1-git-send-email-lstipakov@gmail.com
State Accepted
Headers show
Series [Openvpn-devel] interactive.c: fix usage of potentially uninitialized variable | expand

Commit Message

Lev Stipakov Oct. 8, 2018, 5:35 a.m. UTC 
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 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 <lev@openvpn.net>
---
 src/openvpnserv/interactive.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Comments

Gert Doering Oct. 8, 2018, 6:31 a.m. UTC | #1 
Hi,

On Mon, Oct 08, 2018 at 07:35:54PM +0300, Lev Stipakov wrote:
>  out:
> -    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
Antonio Quartulli Oct. 8, 2018, 6:42 a.m. UTC | #2 
Hi,

On 09/10/18 01:31, Gert Doering wrote:
> Hi,
> 
> On Mon, Oct 08, 2018 at 07:35:54PM +0300, Lev Stipakov wrote:
>>  out:
>> -    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".
> 

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,
Gert Doering Oct. 8, 2018, 6:52 a.m. UTC | #3 
Hi,

On Mon, Oct 08, 2018 at 07:35:54PM +0300, Lev Stipakov wrote:
> @@ -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
diff mbox series

Patch

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 8f92c24..369980b 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;
 
     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;