Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:35291
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain openwall.com designates 195.42.179.200 as permitted sender)
Date: Thu, 7 Feb 2008 14:16:25 +0300
To: Dmitry Stogov <dmitry@zend.com>
Cc: Sara Golemon <saramg_76@yahoo.com>, Stanislav Malyshev <stas@zend.com>,
	Andi Gutmans <andi@zend.com>,
	PHP Internals List <internals@lists.php.net>
Message-ID: <20080207111625.GA27824@openwall.com>
References: <20071209010552.GA12561@openwall.com> <47A849D0.8050508@zend.com> <20080205235055.GA19309@openwall.com> <47AAD95A.8010109@zend.com> <20080207103558.GA27490@openwall.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20080207103558.GA27490@openwall.com>
User-Agent: Mutt/1.4.2.3i
Subject: Re: [PHP-DEV] faster & public domain MD5 implementation
From: solar@openwall.com (Solar Designer)

A followup on my own posting:

> ...and except for adding a note that I'm not the only author (not of the
> modified code).  That's fine, if it's your preference, I don't care much
> either way.

Actually, I am also not the author of some of the code that you're
leaving intact in PHP's md5.c - that's functions make_digest(),
make_digest_ex(), php_if_md5(), and php_if_md5_file().  My contribution
is limited to replacing RSA's implementation of MD5 itself; I did not
contribute replacements for these wrapper functions that are specific
to PHP.

The original PHP's md5.h had "Author: Rasmus Lerdorf" on it, but I am
not sure what that applied to (the header file is small and is hardly
subject to copyright).  The original PHP's md5.c had:

   | Author: Lachlan Roche
...
 * md5.c - Copyright 1997 Lachlan Roche 
 * md5_file() added by Alessandro Astarita <aleast@capri.it>

I am not sure who the code that remains after our changes should be
attributed to.  Maybe you could check the commits history to figure this
out, if it matters.

One thing I'm sure of is that it's OK to drop RSA's copyright and
license notice, because we're replacing that code in its entirety - and
you already did.

> > However new version breaks ext/hash md4. (ext/hash/tests/md4.phpt is 
> > broken).
> 
> That's weird.  Maybe you meant ext/hash/tests/md5.phpt, not md4?  That
> would be somewhat weird, too, as ext/hash/hash_md.c contains its own
> copy of the MD5 code (to be replaced later) - but I suppose there could
> be a symbol clash.

I've recalled that the MD5 code in hash_md.c is within:

#ifdef PHP_HASH_MD5_NOT_IN_CORE
...
#endif /* PHP_HASH_MD5_NOT_IN_CORE */

So if our changes broke the MD5 implementation somehow, it is no
surprise that ext/hash/tests/md5.phpt would fail.

However, I've diff'ed the latest md5.[ch] that you posted against those
I had submitted (and tested) and I failed to notice any obvious bugs
(worse than the php_uint32 vs. size_t issue in PHP_MD5Final(), which
should not cause those tests on short strings to fail).

So this needs further testing on a clean build and, if necessary,
debugging... maybe there's some issue specific to the program at large
that is not obvious when looking just at the changes.

Of course, I've been assuming that php_uint32 is in fact an unsigned
32-bit integer data type.

Alexander