Re: Valgrind Error

[Ingo Schwarze <schwarze@usta.de>]

Hi Kristaps,

Kristaps Dzonsons wrote on Mon, Aug 18, 2014 at 02:13:14AM +0200:

> So to test HEAD (accidentally--I was supposed to be testing
> 1.12.4-rc1 but ran from the wrong directory),

Additional testing is always welcome.  :)

> I wrote a little
> script to run through all the manuals on my system with mandoc via
> valgrind.  It has only found one nit on ALL of them.  On a binary
> file, at that!
[...]
> I tracked this down to passing "\H<nil>" to mandoc_escape().  It
> seems we're not very careful in this function to receiving \0 after
> the initial marker, so enclosed are some check.  I may have missed
> some, so please pass a critical eye over this as well!
> 
> Thoughts?

The middle chunk for \h and friends seems correct.
Indeed, *end must not advance past the \0.

The first (for \f and friends) and third (for \s) seem redundant,
though.  Assume we enter with "\f\0".  By the time we enter the
case, *start and *end point to the \0.  So the inner switch goes
to the default, sets *sz to 1, and goes out of the end of the
big switch with *start and *end both still pointing to the \0,
which is legitimate.  It's the job of the code after the switch
to catch premature end of the argument.  The big switch only catches
the errors related to parsing the argument delimiters, not the
argument itself.

Argument parsing after the switch seems correct.  If term is set,
we go into the while, where - first thing - the switch catches \0.
Otherwise, *sz is compated to strlen(*start), in this case 1 > 0,
so we return right away, too.

I'd say, please commit the middle chunk.

The other two don't add clarity, they merely muddle the point which
code is responsible for parsing what.

Reading the code, i didn't find any other accesses past a \0.
The dangerous cases (having argument delimiters) are \C, \A,
and \N (which already check) and \h (which you correct).
So it looks like an isolated bug in the one case you found.

Yours,
  Ingo


> Index: mandoc.c
> ===================================================================
> RCS file: /usr/vhosts/mdocml.bsd.lv/cvs/mdocml/mandoc.c,v
> retrieving revision 1.85
> diff -u -p -r1.85 mandoc.c
> --- mandoc.c	16 Aug 2014 19:00:01 -0000	1.85
> +++ mandoc.c	17 Aug 2014 23:56:04 -0000
> @@ -150,6 +150,8 @@ mandoc_escape(const char **end, const ch
>  			*start = ++*end;
>  			term = ']';
>  			break;
> +		case '\0':
> +			return(ESCAPE_ERROR);
>  		default:
>  			*sz = 1;
>  			break;
> @@ -199,7 +201,8 @@ mandoc_escape(const char **end, const ch
>  		/* FALLTHROUGH */
>  	case 'x':
>  		if (strchr(" %&()*+-./0123456789:<=>", **start)) {
> -			++*end;
> +			if ('\0' != **start)
> +				++*end;
>  			return(ESCAPE_ERROR);
>  		}
>  		gly = ESCAPE_IGNORE;
> @@ -250,6 +253,8 @@ mandoc_escape(const char **end, const ch
>  			*start = ++*end;
>  			term = '\'';
>  			break;
> +		case '\0':
> +			return(ESCAPE_ERROR);
>  		default:
>  			*sz = 1;
>  			break;
--
 To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv