Re: [evolution-patches] Conditional jump or move depends on uninitialised value(s)

[Jeffrey Stedfast <fejj novell com>]

On Wed, 2007-06-27 at 20:59 +0200, Philip Van Hoof wrote:
> On Wed, 2007-06-27 at 12:29 -0400, Jeffrey Stedfast wrote:
> > What's the bug?
> > 
> > Zeroing the internal buffer doesn't seem like a real fix to me... it
> > might silence the valgrind warnings, but nothing seems to check against
> > '\0', code only either checks inptr < inend or against inptr !=
> > '\n' (inend always gets set to '\n' so there should be no way to compare
> > values past the end of the input afaict)
> > 
> > as far as the other valgrind warnings... well, I can't help but wonder
> > if this is actually a gcc optimization bug?
> > 
> > I don't like the idea of initializing the entire buffer to 0 if it
> > doesn't address a real bug because it may end up hiding real bugs.
> 
> I think the location was looping the pointer until not '\n'. 
> 
> 1352	/* goto the next line */
> 1353	while ((*inptr++)!='\n')
> 1354		;
> 
> I think what valgrind is telling, is that the compare of the value at a
> location in inptr with \n was comparing a value that was never touched
> or written to (so an undefined value). When allocating with 0, that
> would be a '\0'.

but notice that the code only terminates the loop at a '\n', filling the
initial buffer with '\0's galore will not change that.

if the code is broken now, by looping beyond the end of the buffered
input... then filling it with 0's doesn't fix the problem.

filling it with '\n' /might/, but it'd be better to find how it is /not/
being set and fix it appropriately.

I would try this:

in the CamelMimeParser init function (where inbuf/inptr/inend are
initialized), I would add:

inend[0] = '\n';

perhaps that loop is being run before any data is being buffered? (the
code that buffers the next block of input data always terminates with a
'\n' afaict)

I'll look at the code more later if this doesn't solve the problem...

> 
> It's of course a proposal ... I don't actually know what the code is
> supposed to do, I just think the intend of the original programmer, or
> rather his assumption, was that inptr is a buffer with '\0''s

eh... not at all, actually. If there's code that relies on that being
the case for inbuf, then it got broken somehow.

>  and data
> like '\n' and possibly other characters (a line in a mime part?).

yes, there is an optimization hack that always terminates input data
with a '\n' so that while-loop scanning for eoln can be as fast as
possible (by not having to always check inptr < inend until the loop
terminates).

> 
> ps. When uninitialised the data might very well contain for example '\n'
> characters, and a bug might occur here. So valgrind is probably right
> about reporting it.

perhaps, which is why I don't like the memset approach :)


Jeff