Re: Bug#579890: grotty: infinite loop when processing a man page

[Colin Watson]

On Sat, May 01, 2010 at 09:42:04PM -0500, Raphael Geissert wrote:
> grotty appears to enter an infinite loop when processing libwine-dev's 
> (possibly libwine-unstable-dev's too) GetMIMETypeSubKeyA(3w) manpage.
> 
> Reproducing it is as simple as running:
> man --warnings -E UTF-8 -l  GetMIMETypeSubKeyA.3w.gz
> 
> (yep, lintian's check is affected, that's how I noticed it -- not sure if you 
> read the thread on lint-maint.)
> 
> The version on the report is lenny's, but I'm able to reproduce it in sid too 
> (with version 1.20.1-9.)
> I guess that running grotty under the effects of a fuzzer would reveal more 
> bugs.

Thanks.  Here's a reduced test case (run with 'groff -Tutf8 -mandoc'):

  .TH GetMIMETypeSubKeyA 3w "Jun 2009" "Wine API" "Wine API"
  .SH NAME
  \fBGetMIMETypeSubKeyA\fR (SHLWAPI.328)
  .SH NOTES
  .PP
  The base path for the key is \fB"MIME\Database\Content Type\"\fR

There are two bugs here.  The simpler one to fix is the bug in the
manual page (CCed address@hidden for this).  It's using \ to
mean a literal backslash, but actually \ introduces a groff escape; \D
emits a drawing command while \C typesets a glyph by name.  This line
should instead read:

  The base path for the key is \fB"MIME\eDatabase\eContent Type\"\fR

The infinite loop in grotty is a bit harder.  What's happening is that
the \D escape emits ditroff commands fairly directly, and in this
particular case you end up with a line that just contains 'Dt', without
the required argument.  In fact, you can reproduce this infinite loop
with just the following grotty input:

  x T utf8
  x res 240 24 40
  x init
  p1
  Dt

The following patch would turn this into a fatal error instead, which
isn't ideal either but is certainly better than an infinite loop.
However, I don't know this code very well and would appreciate review.

2010-05-04  Colin Watson  <address@hidden>

        * src/libs/libdriver/input.cpp (get_integer_arg): Emit a fatal error
        on a non-integer argument, bringing the code into line with the
        behaviour documented in the header comment.
        (get_possibly_integer_args): Terminate the loop on a non-integer
        argument.
        (next_arg_begin): Return newline or EOF after emitting the
        corresponding error, rather than continuing on to the next line.

Index: b/src/libs/libdriver/input.cpp
===================================================================
--- a/src/libs/libdriver/input.cpp
+++ b/src/libs/libdriver/input.cpp
@@ -790,7 +790,7 @@
     c = get_char();
   }
   if (!isdigit((int) c))
-    error("integer argument expected");
+    fatal("integer argument expected");
   while (isdigit((int) c)) {
     buf.append(c);
     c = get_char();
@@ -879,6 +879,7 @@
       break;
     default:
       error("integer argument expected");
+      done = true;
       break;
     }
   }
@@ -946,7 +947,7 @@
     case '\n':
     case EOF:
       error("missing argument");
-      break;
+      return c;
     default:                   // first essential character
       return c;
     }

Thanks,

-- 
Colin Watson                                       address@hidden