Valgrind Error

Valgrind Error

[Kristaps Dzonsons]

[-- Attachment #1: Type: text/plain, Size: 1888 bytes --]

Hi,

So to test HEAD (accidentally--I was supposed to be testing 1.12.4-rc1 
but ran from the wrong directory), 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!

Invalid read of size 1
    at 0x100017CC4: term_word (term.c:421)
    by 0x10000F4A1: print_man_node (man_term.c:974)
    by 0x10000F38C: print_man_nodelist (man_term.c:1042)
    by 0x10000F3BC: print_man_nodelist (man_term.c:1045)
    by 0x10000F3BC: print_man_nodelist (man_term.c:1045)
    by 0x10000F3BC: print_man_nodelist (man_term.c:1045)
    by 0x10000F3BC: print_man_nodelist (man_term.c:1045)
    by 0x10000F3BC: print_man_nodelist (man_term.c:1045)
    by 0x10000F3BC: print_man_nodelist (man_term.c:1045)
    by 0x10000F3BC: print_man_nodelist (man_term.c:1045)
    by 0x10000F3BC: print_man_nodelist (man_term.c:1045)
    by 0x10000F3BC: print_man_nodelist (man_term.c:1045)
  Address 0x100082e12 is 0 bytes after a block of size 306 alloc'd
    at 0xC658: malloc (vg_replace_malloc.c:295)
    by 0x28E358: strdup (in /usr/lib/system/libsystem_c.dylib)
    by 0x100046164: mandoc_strdup (mandoc_aux.c:102)
    by 0x10003D89B: roff_strdup (roff.c:2233)
    by 0x100025636: man_word_alloc (man.c:308)
    by 0x100024E26: man_ptext (man.c:449)
    by 0x100024316: man_parseln (man.c:126)
    by 0x100047F1D: mparse_buf_r (read.c:562)
    by 0x10004665C: mparse_parse_buffer (read.c:718)
    by 0x1000467A3: mparse_readfd (read.c:764)
    by 0x10001E2C4: parse (main.c:308)
    by 0x10001DA08: main (main.c:244)

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?

Best,

Kristaps

[-- Attachment #2: mandoc_term.diff --]
[-- Type: text/plain, Size: 1488 bytes --]

? Makefile.depend.patch
? Makefile.local
? apropos
? article-template.xml
? article1.html
? article1.xml
? cgi.h
? config.h
? config.log
? demandoc
? foo.1
? foo.1.html
? foo.man
? foo.ps
? foo.sh
? gluPerspective.3
? gluPerspective.html
? hspaces.diff
? html5.diff
? html5_cgi.diff
? html5_test2.diff
? itcrash.diff
? makewhatis
? mandoc
? mandoc.dSYM
? mandoc.html
? mandoc_term.diff
? mandocdb
? patch
? preconv
? querystring.diff
? roff_res_charwidth.patch
? scale.diff
? test.1
? test.1.html
? test.1.ps
? test.2
? test.2.ps
? test.ps
? testm.ps
? testn.ps
? unit_charwidth.patch
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;

Re: Valgrind Error

[Ingo Schwarze]

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

More Valgrind Errors

[Kristaps Dzonsons]

Ingo,

Committed, thanks!

Another mystery, still on HEAD.  (And in the many Mac OSX manuals, this 
is the end of what I found.)  Consider the following manual (reduced 
from groff.7 around line 404, where the leak was originally found):

.TH GROFF 7 "16 February 2005" "Groff Version 1.19.2"
.SH NAME
groff \- a short reference for the GNU roff language
.SH DESCRIPTION
.RS
.P
.RE
hello, world

valgrind notes that the RS body is never freed ("indirectly lost", in 
its parlance).  In looking closely, the -Ttree output just isn't quite 
right (clipped for the screen):

root (root) 0:1
	SH (block) *2:2
		SH (block-head) 2:2
			NAME (text) 2:5
		SH (block-body) 2:2
			groff \- ... (text) *3:1
	SH (block) *4:2
		SH (block-head) 4:2
			DESCRIPTION (text) 4:5
		SH (block-body) 4:2
			RS (block) *5:2
				hello, world (text) *8:1

I'm afraid I don't have the time to dive into this right now--any ideas 
on this behaviour?

Meanwhile, I'm running through the manuals again and will then do the 
same with the 1.12 branch.

Best,

Kristaps
--
 To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv

Re: More Valgrind Errors

[Ingo Schwarze]

Hi Kristaps,

Kristaps Dzonsons wrote on Mon, Aug 18, 2014 at 12:21:20PM +0200:

> Another mystery, still on HEAD.  (And in the many Mac OSX manuals,
> this is the end of what I found.)  Consider the following manual
> (reduced from groff.7 around line 404, where the leak was originally
> found):
> 
> .TH GROFF 7 "16 February 2005" "Groff Version 1.19.2"
> .SH NAME
> groff \- a short reference for the GNU roff language
> .SH DESCRIPTION
> .RS
> .P
> .RE
> hello, world
> 
> valgrind notes that the RS body is never freed ("indirectly lost",
> in its parlance).  In looking closely, the -Ttree output just isn't
> quite right (clipped for the screen):
> 
> root (root) 0:1
> 	SH (block) *2:2
> 		SH (block-head) 2:2
> 			NAME (text) 2:5
> 		SH (block-body) 2:2
> 			groff \- ... (text) *3:1
> 	SH (block) *4:2
> 		SH (block-head) 4:2
> 			DESCRIPTION (text) 4:5
> 		SH (block-body) 4:2
> 			RS (block) *5:2
> 				hello, world (text) *8:1
> 
> I'm afraid I don't have the time to dive into this right now--any
> ideas on this behaviour?

Fixed, in all branches, thanks!

> Meanwhile, I'm running through the manuals again and will then do
> the same with the 1.12 branch.

Good, i'm looking forward to the results

Yours,
  Ingo
--
 To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv