Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:35288
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 13:35:58 +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: <20080207103558.GA27490@openwall.com>
References: <20071209010552.GA12561@openwall.com> <47A849D0.8050508@zend.com> <20080205235055.GA19309@openwall.com> <47AAD95A.8010109@zend.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <47AAD95A.8010109@zend.com>
User-Agent: Mutt/1.4.2.3i
Subject: Re: [PHP-DEV] faster & public domain MD5 implementation
From: solar@openwall.com (Solar Designer)

Hi Dmitry,

On Thu, Feb 07, 2008 at 01:11:38PM +0300, Dmitry Stogov wrote:
> I've updated the files according to your notes, except 
> php_uint32/MD5_u32plus.

...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.

> 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.

Did you check that PHP md5() still works correctly after your changes?

(The patch I had submitted was fine in this respect, passing my tests.)

Some more comments on the files:

>  *  * This is an OpenSSL-compatible implementation of the RSA Data Security,
>  *   * Inc. MD5 Message-Digest Algorithm (RFC 1321).
>  *    *
>  *     * Written by Solar Designer <solar at openwall.com> in 2001, and placed
>  *      * in the public domain.  There's absolutely no warranty.
>  *       *
>  *        * This differs from Colin Plumb's older public domain implementation in
...
>  *               * and avoid compile-time configuration.

Why are you re-formatting these comments in such a weird way, making
lines exceed 80 chars for long comments?

> PHPAPI void PHP_MD5Final(unsigned char *result, PHP_MD5_CTX *ctx)
> {
> 	php_uint32 used, free;

These should be size_t, not php_uint32.

> PHPAPI void make_digest_ex(char *md5str, const unsigned char *digest, int len);

You could want to change this to "size_t len" as well, although this
function existed before and used "int".  Obviously, if you do change
it, you need to do so in both files and perhaps also in some callers.
Maybe this should be done with a separate commit (a change not directly
related to replacing the MD5 implementation).

Thanks,

-- 
Alexander Peslyak <solar at openwall.com>
GPG key ID: 5B341F15  fp: B3FB 63F4 D7A3 BCCC 6F6E  FC55 A2FC 027C 5B34 1F15
http://www.openwall.com - bringing security into open computing environments