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
--
To UNSUBSCRIBE, email to [EMAIL PROTECTED]
with a subject of "unsubscribe". Trouble? Contact [EMAIL PROTECTED]
