Bug#317524: infinite loop when using charset_default=iso-8859-1

[David Relson]

On Sat, 5 Nov 2005 03:14:38 +0100
Elmar Hoffmann wrote:

> Hi David,
> 
> on Fri, Nov 04, 2005 at 19:22:38 -0500, you wrote:
> 
> > The attached patch fixes both problems.
> 
> But unfortunately introduces new ones:
> 
> >  static char *get_string(const char *name, const char *arg)
> >  {
> >      char *s = xstrdup(arg);
> > +
> > +    /* scan option to delete comment (after '#') and preceding
> > whitespace */
> > +    char *t = s;
> 
> t initially points to s, a copy of arg...
> 
> > +    bool quote = false;
> > +    for (t = s; *t != '\0' ; t += 1)
> > +    {
> > +       char c = *t;
> > +       if (c == '"')
> > +           quote ^= true;
> > +       if (!quote && c == '#') {
> > +           *t-- = '\0';
> 
> ...if arg starts with a '#' (ie. something like "option=# foo" in the
> config), t will now point one byte before the beginning of s...
> 
> > +           while (isspace(*t))
> 
> ...and thus a faulty memory access will happen here.
> 
> > +               *t-- = '\0';
> > +           break;
> > +       }
> > +    }
> 
> The attached fixed version of the patch avoids this (and further code
> duplication) by using the existing remove_comment() function, which
> already is used by other get_*() functions.
> 
> The other problem is that init_charset_table_iconv() is not the only
> place bf_iconv_open() is used without checking for a result of -1.
> text_decode() in lexer.c contains the lines
>             cd = bf_iconv_open( charset_unicode, charset );
>             iconvert_cd(cd, &src, buf);
> and iconvert_cd() checks for cd == NULL only.
> 
> Thus I think it makes more sense to fix bf_iconv_open() itself to
> always return NULL on failure, like in the attached patch.
> 
> elmar

Nice work, elmar!

Your observations about the code are very good, though not totally
correct.  If you look at function process_config_option(), you'll see
that the original config line is duplicated and split at the '='
character (which has been replaced by '\0').  The "*t--" code is
applied to the part after '=' and the loop to erase whitespace will
never backup to the part before the '='.  When I made the quick change
to get_string(), I hadn't fully analyzed the situation and had assumed
(rather than determined) that all was safe.

In any case, using remove_comment() is the better solution, as is using
xd=NULL.  CVS now has the new version.  In honor of your contribution
to the code base, your name has been added to the AUTHORS list.

Well done!

David