[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: unitialised memory read, a suspicious MD5 checksum?

From: <cmpilato_at_collab.net>
Date: 2002-05-16 19:38:01 CEST

Philip Martin <philip@codematters.co.uk> writes:

> cmpilato@collab.net writes:
>
> > Philip Martin <philip@codematters.co.uk> writes:
> >
> > > After a little investigation it appears to be a suspicious md5
> > > checksum. Running the commit without valgrind and setting a breakpoint
> > > in svn_fs__unparse_representation_skel at fs_skels.c:668 I see
> >
> > Yeah, we simply aren't using the MD5 checksum field right now -- it's
> > one of the many things on the "to-do" list.
>
> Ok, I still think we might have a bug in the current code.
>
> Looking at svn_fs__rep_deltify() it calls svn_txdelta() to create
> txdelta_stream. This does not initialise the digest member. Later
> svn_fs__rep_deltify() calls svn_txdelta_md5_digest() to retrieve the
> digest, it has still not been initialised. It gets memcpy'd into a
> chunk, and later memcpy'd into a skel. This is the memory that is
> used in use_implict() to query the skel_char_type table.

Okay, tracing through this code now. svn_txdelta() does indeed create
an initialized `digest' member, though that digest should be in safely
allocated memory (just full of garbage). From there on, that digest
is only used in two ways: copied to other digests, and marshaled into
and out of skels.

The marshaling is where you're seeing code that is making decisions
based on the contents of `digest', but this is far from dangerous -- I
mean, a *real* MD5 digest would suffer the same way, more or less
depending on how the bits fall. Think of use_implicit as a way of
choosing whether or not to base64 encode or uuencode some data -- it
doesn't really matter the decision made, because the original data
(garbage, in this case) can still be unmarshaled later (at which time
it will be used for nothing).

So, this uninitialized data should not be dangerous, but is an
annoyance that causes warnings in your boundschecker. I'd say just
use apr_pcalloc (instead of apr_palloc) in svn_txdelta() and be done
with it. :-)

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu May 16 19:39:14 2002

This is an archived mail posted to the Subversion Dev mailing list.