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(at)commandprompt(dot)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