Nmap Development mailing list archives
Re: Bug when loading command line exclude list
From: David Fifield <david () bamsoftware com>
Date: Sun, 14 Jun 2009 17:04:56 -0600
On Sun, May 24, 2009 at 04:27:43PM -0600, David Fifield wrote:
On Tue, May 19, 2009 at 02:10:39PM -0400, Will Cladek wrote:I found a bug in the load_exclude function in targets.cc. Here's the affected code: else { /* If we are parsing command line, load the exclude file from the string */ p_acBuf=strdup(szExclude); pc=strtok(p_acBuf, ","); while (NULL != pc) { if(excludelist[i].parse_expr(pc,o.af()) == 0) { if (o.debugging >1) error("Loaded exclude target of: %s", pc); ++i; } /* This is a totally cheezy hack, but since I can't use strtok_r... * If you can think of a better way to do this, feel free to change. * As for now, we will reset strtok each time we leave parse_expr */ { int hack_i; char *hack_c = strdup(szExclude); pc=strtok(hack_c, ","); for (hack_i = 0; hack_i < i; hack_i++) pc=strtok(NULL, ","); free(hack_c); } } } The problem is in the "cheezy hack", hack_c is duplicated from the string of excluded hosts and pc is made to point to the next host in that string. But then hack_c is freed before pc, which points to that freed bit of memory, is used on the next iteration of the while loop, occasionally causing fatal errors.Thanks for finding this. I was trying to figure out why this code is so complicated, then I remembered that TargetGroup::parse_expr used to use the strtok function too. As you know, strtok has secret internal state so it can't be used concurrently by two different functions. That's why this code has to "reset" it. But Brandon Enright rewrote parse_expr not to use strtok in r12613, in response to a different bug (also related to the use of strtok). Can you send another patch that either uses a straightforward strtok loop (should be much shorter), or (better) does the same thing without calling strtok?
I forgot to reply this request back to Will so he may not have seen it. I changed the code to use a straightforward strtok loop. The load_exclude function needs to be rewritten from scratch to be simpler and clearer. As it is, it parses the input file or command line argument twice, the first time to get the number of entries and allocate a buffer, the second to fill in the buffer. This makes it hard to change how it works and could cause a buffer overflow in the future if someone updates one loop but not the other. A tested replacement is welcome. David Fifield _______________________________________________ Sent through the nmap-dev mailing list http://cgi.insecure.org/mailman/listinfo/nmap-dev Archived at http://SecLists.Org
Current thread:
- Bug when loading command line exclude list Will Cladek (May 19)
- Re: Bug when loading command line exclude list David Fifield (May 24)
- Re: Bug when loading command line exclude list David Fifield (Jun 14)
- Re: Bug when loading command line exclude list David Fifield (May 24)