Hi,
On Tue, Nov 12, 2024 at 12:28:53PM +0900, Michael Paquier wrote:
> On Mon, Nov 11, 2024 at 05:07:51PM +0000, Bertrand Drouvot wrote:
> > To handle the "what about the len check if the function is not inlined?", I
> > can't think about a good approach.
>
> FWIW, my choice would be to not over-engineer things more than what's
> in v10 posted at [1], hence do something without the exception case
> where the size is less than 64b.
I think that the 64b len check done in v11 is mandatory for safety reasons.
1. First reason:
"
for (; p < aligned_end - (sizeof(size_t) * 7); p += sizeof(size_t) * 8)
"
The loop above reads 64 bytes at once, so would read beyond the memory area bounds
if len < 64: That could cause crash or read invalid data.
It's observed in [1] (using the godbolt shared in [2]), where we can see:
"
movdqu xmm2, XMMWORD PTR [rdi+16]
movdqu xmm1, XMMWORD PTR [rdi+32]
movdqu xmm3, XMMWORD PTR [rdi+48]
"
while the struct size is 16 bytes (so we are reading 48 bytes beyond it).
2. Second reason
"
const unsigned char *aligned_end = (const unsigned char *)
((uintptr_t) end & (~(sizeof(size_t) - 1)));
"
aligned_end could be beyond the end for len < 8, so that we could read
invalid data or crash here:
"
for (; p < aligned_end; p += sizeof(size_t)) {
"
The len < 8 check is covered into the len < 64 check, so only the 64b check is
needed.
[1]: https://www.postgresql.org/message-id/Zy7hyG8JUMC5P2T3%40ip-10-97-1-34.eu-west-3.compute.internal
[2]: https://www.postgresql.org/message-id/CAApHDvp2jx_%3DpFbgj-O1_ZmzP9WOZKfwLzZrS_%3DZmbsqMQQ59g%40mail.gmail.com
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
