| Message ID | 1539022378-24485-1-git-send-email-lstipakov@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel,v2] interactive.c: fix usage of potentially uninitialized variable | expand |
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 <<a href="mailto:lstipakov@gmail.com" target="_blank">lstipakov@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">From: Lev Stipakov <<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>><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 <<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>><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 "return err".<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, &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, &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
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; 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)