fix(postgres) use signed int for length prefix in PgCopyIn

[joeydewaal]

While looking into #3696 I noticed that CopyData uses an i32 to represent the length prefix. However, the AsyncRead impl uses an u32.

sqlx/sqlx-postgres/src/copy.rs

Line 233 in a83395a

let read32 = u32::try_from(read)

[abonander]

Yeah, ultimately this doesn't really matter because Postgres is hardcoded to reject messages above a certain size depending on the message type:

• CopyData uses PQ_LARGE_MESSAGE_LIMIT: https://github.com/postgres/postgres/blob/a5579a90af05814eb5dc2fd5f68ce803899d2504/src/backend/tcop/postgres.c#L435
• PQ_LARGE_MESSAGE_LIMIT is MaxAllocSize - 1: https://github.com/postgres/postgres/blob/master/src/include/libpq/libpq.h#L31
• MaxAllocSize is 1 GiB - 1: https://github.com/postgres/postgres/blob/a5579a90af05814eb5dc2fd5f68ce803899d2504/src/include/utils/memutils.h#L40

This includes the 4-byte length prefix, which is why it's actually 1 GiB - 5 for the message contents.

Writing a u32 instead of an i32 would just overflow to negative, which it also rejects. So while this is a tiny correctness improvement, it won't fix the bug.

The reason why #3440 is so dangerous is that unchecked casts from usize could overflow back around to a valid value, producing a buffer underflow attack.

I commented on #3696 (comment) for what the fix should look like.

[joeydewaal]

Writing a u32 instead of an i32 would just overflow to negative, which it also rejects. So while this is a tiny correctness improvement, it won't fix the bug.

I figured that's why postgres would still work with small payloads but would hang with big ones.

Thanks for the detailed reply, I'll try and come up with a more correct solution for this PR and #3696.