Re: Snapshot synchronization, again...

On 19.02.2011 02:41, Joachim Wieland wrote:
> On Fri, Feb 18, 2011 at 8:57 PM, Alvaro Herrera
> <alvherre@commandprompt.com>  wrote:
>> 1. why are you using the expansible char array stuff instead of using
>> the StringInfo facility?
>>
>> 2. is md5 the most appropriate digest for this?  If you need a
>> cryptographically secure hash, do we need something stronger?  If not,
>> why not just use hash_any?
>
> We don't need a cryptographically secure hash.

Really? The idea of the hash is to prevent you from importing arbitrary 
snapshots into the system, allowing only copying snapshots that are in 
use by another backend. The purpose of the hash is to make sure the 
snapshot the client passes in is identical to one used by another backend.

If you don't use a cryptographically secure hash, it's easy to construct 
a snapshot with the same hash as an existing snapshot, with more or less 
arbitrary contents.

This also makes me worried:
> +
> +     /*
> +      * Verify that the checksum matches before doing any more work. We will
> +      * later verify again to make sure that the exporting transaction has not
> +      * yet terminated by then. We don't want to optimize this into just one
> +      * verification call at the very end because the instructions that follow
> +      * this comment rely on a sane format of the textual snapshot data. In
> +      * particular they assume that there are not more XactIds than
> +      * advertised...
> +      */

It sounds like you risk a buffer overflow if the snapshot is bogus, 
which makes a collision-resistant hash even more important.

I know this was discussed already, but I don't much like using a hash 
like this. We should find a way to just store the whole snapshot in 
shared memory, or even a temporary file if we want to skimp on shared 
memory. It's generally better to not rely on cryptography when you don't 
have to.

--   Heikki Linnakangas  EnterpriseDB   http://www.enterprisedb.com