Re: [nmap-svn] r32398 - in nmap: ncat nsock/src

[Henri Doreau <henri.doreau () gmail com>]

2013/9/26  <commit-mailer () nmap org>:
> Author: david
> Date: Thu Sep 26 07:17:08 2013
> New Revision: 32398
>
> Log:
> Use tempfile in place of tempnam.
>
> To avoid new GCC warnings about tempnam:
> ncat_connect.c:789: warning: the use of `tempnam' is dangerous, better use `mkstemp'
>
> Doing things this way has the same race condition as tempnam did,
> because we are unlinking the file before binding it. (The race window is
> smaller now.) The file must not exist before binding the Unix socket, or
> else you get an "address already in use" error. Unlinking before binding
> is the same thing that netcat-openbsd does. See this earlier thread:
> http://seclists.org/nmap-dev/2012/q4/336.
[...]
> -  nsock_log_info(ms, "Binding to %s (IOD #%li)", get_localaddr_string(iod), iod->id);
> +  path = get_localaddr_string(iod);
> +  unlink(path);
> +  nsock_log_info(ms, "Binding to %s (IOD #%li)", path, iod->id);
>    rc = bind(iod->sd, (struct sockaddr *)&iod->local, (int) iod->locallen);

Hi David,

I have a couple comments on this patch too.

Calling unlink() unconditionally seems wrong to me, we should only do
it when dealing with unix sockets. I wouldn't like nmap to delete my
files just because they're called "127.0.0.1:8080" or something...

It's almost cosmetic but I would also move the nsock_log statement
before the unlink, so that it's very obvious that the race window is
minimal.

Regards

-- 
Henri
_______________________________________________
Sent through the dev mailing list
http://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/