New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Makes sure that proxy_auth is properly disposed #975

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@landgraf

landgraf commented Aug 18, 2017

In case of error in proxy connection initialization proxy_auth pointer
goes out of scope and memory leak reported by static analizer tool.
While this is not critical because of application exit the fix is trivial.

Show outdated Hide outdated ncat/ncat_connect.c
@@ -695,21 +695,18 @@ static int do_proxy_socks5(void)
if (send(sd, (char *) &socks5msg, len, 0) < 0) {
loguser("Error: proxy request: %s.\n", socket_strerror(socket_errno()));
close(sd);
return -1;
goto error;

This comment has been minimized.

@kdudka

kdudka Aug 18, 2017

All the goto error; statements are indented by tabs whereas the surrounding code uses spaces for indentation.

@kdudka

kdudka Aug 18, 2017

All the goto error; statements are indented by tabs whereas the surrounding code uses spaces for indentation.

Makes sure that proxy_auth is properly disposed of if the proxy conne…
…ction fails.

In case of error in proxy connection initialization proxy_auth pointer
goes out of scope and memory leak reported by static analizer tool.
While this is not critical because of application exit the fix is trivial.
@landgraf

This comment has been minimized.

Show comment
Hide comment
@landgraf

landgraf Aug 18, 2017

Fixed (in amended commit)

landgraf commented Aug 18, 2017

Fixed (in amended commit)

Show outdated Hide outdated ncat/ncat_connect.c
}
return(sd);
error:
if (proxy_auth != NULL)
free(proxy_auth);

This comment has been minimized.

@kdudka

kdudka Aug 18, 2017

I think you should free(proxy_auth) also when no error occurs. This is not a problem introduced by your patch but still worth fixing.

@kdudka

kdudka Aug 18, 2017

I think you should free(proxy_auth) also when no error occurs. This is not a problem introduced by your patch but still worth fixing.

Show outdated Hide outdated ncat/ncat_connect.c
@@ -788,6 +788,8 @@ static int do_proxy_socks5(void)
goto error;
}
free(proxy_auth);

This comment has been minimized.

@kdudka

kdudka Aug 18, 2017

This will cause a double free() if you jump to error: from now on. Either put this bellow all the gotos, or put proxy_auth = NULL; immediately after the first free().

@kdudka

kdudka Aug 18, 2017

This will cause a double free() if you jump to error: from now on. Either put this bellow all the gotos, or put proxy_auth = NULL; immediately after the first free().

This comment has been minimized.

@landgraf

landgraf Aug 18, 2017

Oops. Sorry. fixed

@landgraf

landgraf Aug 18, 2017

Oops. Sorry. fixed

@kdudka

kdudka approved these changes Aug 18, 2017

Looks good.

@nnposter

This comment has been minimized.

Show comment
Hide comment
@nnposter

nnposter Aug 21, 2017

I see the issue and the fix makes sense but there is some other potentially suspect code in the vicinity so we might end up resolving it differently. Thank you for the note.

nnposter commented Aug 21, 2017

I see the issue and the fix makes sense but there is some other potentially suspect code in the vicinity so we might end up resolving it differently. Thank you for the note.

@nnposter

This comment has been minimized.

Show comment
Hide comment
@nnposter

nnposter Sep 23, 2017

Resolved in r37005 (6e83dc6)

nnposter commented Sep 23, 2017

Resolved in r37005 (6e83dc6)

@nmap-bot nmap-bot closed this in 260d009 Sep 24, 2017

@landgraf landgraf deleted the landgraf:proxy_auth_leak branch Sep 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment