[Asterisk-Dev] Re: asterisk enum.c,1.20,1.20.2.1

[Tony Mountifield]

In article <20050317002356.0CA6D3C2BD at lists.digium.com>,
 <russell at lists.digium.com> wrote:
> Update of /usr/cvsroot/asterisk
> In directory mongoose.digium.com:/tmp/cvs-serv27398
> 
> Modified Files:
>       Tag: v1-0
> 	enum.c 
> Log Message:
> fix TXTCIDName (bug #3681)

This fix looks a bit broken to me. I've reviewed the mentioned bug,
and I see that it's the same one that gave rise to the awful
(char)"\0" business the other day! I can't add a bugnote as it is
now closed.

I'm a bit alarmed that changes are accepted into CVS so readily from
anyone who admits their C coding is "rusty".

See below for comments.

> Index: enum.c
> ===================================================================
> RCS file: /usr/cvsroot/asterisk/enum.c,v
> retrieving revision 1.20
> retrieving revision 1.20.2.1
> diff -u -d -r1.20 -r1.20.2.1
> --- enum.c	14 Jul 2004 07:44:19 -0000	1.20
> +++ enum.c	17 Mar 2005 00:19:23 -0000	1.20.2.1
> @@ -244,16 +244,31 @@
>  	printf("ENUMTXT Called\n");
>  #endif
>  
> -	if (answer != NULL) {
> -		c->txtlen = strlen(answer);
> -		strncpy(c->txt, answer, sizeof(c->txt) - 1);
> -		c->txt[sizeof(c->txt) - 1] = 0;
> -		return 1;
> -	} else {
> +	if (answer == NULL) {
>  		c->txt = NULL;
>  		c->txtlen = 0;
>  		return 0;
>  	}
> +
> +	/* skip over first byte, as for some reason it's a vertical tab character */
> +	answer += 1;
> +	len -= 1;

:-O surely the "some reason" should be identified before deciding on a patch!

> +
> +	/* answer is not null-terminated, but should be */

That is another clue. In fact the string in question was "\x0BJared Smith".
Just looking at that, it is obvious that the 0x0B byte is a length field
with value 11, because there are 11 characters in "Jared Smith".

I haven't looked at the original code yet, the patch doesn't show
where len comes from, but any termination byte should be written in a
place determined by that first length byte.

Or better still, just use the length byte to control the strncpy, and
terminate the string once it is in c->txt.

> +	/* this is safe to do, as answer has extra bytes on the end we can 
> +           safely overwrite with a null */
> +	answer[len] = '\0';
> +	/* now increment len so that len includes the null, so that we can
> +	   compare apples to apples */
> +	len +=1;
> +
> +	/* finally, copy the answer into c->txt */
> +	strncpy(c->txt, answer, len < c->txtlen ? len-1 : (c->txtlen)-1);
> +	
> +	/* just to be safe, let's make sure c->txt is null terminated */
> +	c->txt[(c->txtlen)-1] = '\0';
> +
> +	return 1;
>  }
>  
>  static int enum_callback(void *context, u_char *answer, int len, u_char *fullanswer)
> 
> _______________________________________________
> Asterisk-Cvs mailing list

I don't actually use that part of Asterisk, so hopefully someone who
does can address this issue.

Cheers
Tony
-- 
Tony Mountifield
Work: tony at softins.co.uk - http://www.softins.co.uk
Play: tony at mountifield.org - http://tony.mountifield.org