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