Re: Bug when loading command line exclude list

[David Fifield <david () bamsoftware com>]

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?

David Fifield

_______________________________________________
Sent through the nmap-dev mailing list
http://cgi.insecure.org/mailman/listinfo/nmap-dev
Archived at http://SecLists.Org