[Openvpn-devel] interactive.c: fix usage of potentially uninitialized variable
Commit Message
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
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
@@ -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;