[m-rev.] for review: fix bugs in mdb breakpoint procedure specification

[Fergus Henderson]

On 11-Feb-2002, Simon Taylor <stayl at cs.mu.OZ.AU> wrote:
> 
> +++ trace/mercury_trace_tables.c	10 Feb 2002 15:30:38 -0000
> @@ -39,6 +39,8 @@
>  			*file_layout, int line,
>  			MR_file_line_callback callback_func, int callback_arg);
>  
> +static bool MR_parse_trailing_number(char *str, char **s, int *n);

Please use more informative argument names here.

> @@ -218,9 +220,11 @@
>  MR_parse_proc_spec(char *str, MR_Proc_Spec *spec)
...
> +	/*
> +	** Check for the optional trailing arity and mode number. 
> +	** This also checks for filename:linenumber breakpoint specifiers.
> +	*/
> +	end = str + len - 1;
> +	if (MR_parse_trailing_number(str, &end, &n)) {
> +		if (end < str) {

This test is dangerous -- if end < str, then undefined behaviour
may have already occurred (see below).

> +	/* Search backwards for the end of the final module qualifier. */
> +	while (end >= str) {

Likewise.

> +static bool
> +MR_parse_trailing_number(char *start, char **end, int *n)
> +{
> +	bool found_digit = FALSE;
> +	int power_of_10 = 1;	
> +
> +	*n = 0;
> +	while (*end >= start && MR_isdigit(**end)) {
> +		found_digit = TRUE;
> +		*n += power_of_10 * (**end - '0');
> +		power_of_10 *= 10;
> +		(*end)--;
> +	}
> +	return found_digit;
> +}

This routine is dangerous: it requires `start'
to point to at least one character *past* the start of the
string.  Otherwise, if the string contains only digits,
then this routine will set `end' to `start - 1', which
in standard C will result in undefined behaviour
if `start' points to the start of the string.

Now, in fact in this case I think it is probably safe, since
the memory to hold the strings passed to this routine is
allocated once for each line, rather than for each word,
and `start' will never point to the start of a line...
however, relying on that would make the software very fragile.
At a minimum the requirement should be documented in all
functions that depend on it... but it would be better to
rewrite the code so as to not depend on this requirement.

Apart from that, this change looks fine.  But I'd like to see another
diff for trace/mercury_trace_tables.c when you've addressed that issue.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
The University of Melbourne         |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.
--------------------------------------------------------------------------
mercury-reviews mailing list
post:  mercury-reviews at cs.mu.oz.au
administrative address: owner-mercury-reviews at cs.mu.oz.au
unsubscribe: Address: mercury-reviews-request at cs.mu.oz.au Message: unsubscribe
subscribe:   Address: mercury-reviews-request at cs.mu.oz.au Message: subscribe
--------------------------------------------------------------------------