Skip to content

Makes sure that proxy_auth is properly disposed - #975

Closed
landgraf wants to merge 2 commits into
nmap:masterfrom
landgraf:proxy_auth_leak
Closed

landgraf wants to merge 2 commits into
nmap:masterfrom
landgraf:proxy_auth_leak

Conversation

@landgraf

Copy link
Copy Markdown

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.

Comment thread ncat/ncat_connect.c Outdated
loguser("Error: proxy request: %s.\n", socket_strerror(socket_errno()));
close(sd);
return -1;
goto error;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

…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

Copy link
Copy Markdown
Author

Fixed (in amended commit)

Comment thread ncat/ncat_connect.c Outdated
return(sd);
error:
if (proxy_auth != NULL)
free(proxy_auth);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread ncat/ncat_connect.c Outdated
goto error;
}

free(proxy_auth);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. Sorry. fixed

@kdudka kdudka left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@nnposter

Copy link
Copy Markdown

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

Copy link
Copy Markdown

Resolved in r37005 (6e83dc6)

@nmap-bot nmap-bot closed this in 260d009 Sep 24, 2017
@landgraf
landgraf deleted the proxy_auth_leak branch September 25, 2017 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants