Re: Bug 29863 - Segmentation fault in memcmp-sse2.S if memory contents can concurrently change

[Noah Goldstein <goldstein.w.n@gmail.com>]

On Tue, Dec 13, 2022 at 1:20 PM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Noah Goldstein via Libc-alpha:
>
> > Is this something we have to support? I believe other functions /
> > implementations of memcmp will suffer from a similar bug.
>
> Of course the crash is by no means deterministic, so I'm not sure how
> useful it is to detect application bugs.  Maybe papering over the
> application bug is the right approach here.
>
> On the other hand, I really don't see how such a racing memcmp call
> could deliver any useful information whatsoever.  The result will always
> be arbitrary in practice.  So I hope such application bugs are really
> rare.

The usecase in the bugzilla is optimistic reads in concurrent databases.

>
> > The fix:
> > https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/x86_64/multiarch/memcmp-sse2.S;h=afd450d0206d6633da9fbc4607a7fa6aeb4e137c;hb=HEAD#l46
> > ```
> > -#   define SIZE_OFFSET (CHAR_PER_VEC * 2)
> > +#   define SIZE_OFFSET 0
> > ```
>
> How costly is this change?  I would have thought about ANDing the offset
> so that it is always in range (but maybe it will stil result in a page
> crossing, I don't really know how this works).

Its a bit expensive b.c it misaligned a lot of hot paths.

A better fix (untested) is probably:
```
@@ -308,7 +308,7 @@ L(ret_nonzero_vec_end_0):
  setg %dl
  leal -1(%rdx, %rdx), %eax
 #  else
- addl %edx, %eax
+ addq %rdx, %rax
  movzbl (VEC_SIZE * -1 + SIZE_OFFSET)(%rsi, %rax), %ecx
  movzbl (VEC_SIZE * -1 + SIZE_OFFSET)(%rdi, %rax), %eax
  subl %ecx, %eax

```
The issue is the 32-bit negative number becomes a very large
unsigned 64-bit number. If we just use 64-bit addition the
issue goes away (I think at least).

I'm okay with this change going in (assuming it works).

This fix can be a "happy accident" that this unsupported
usage no longer causes a sig-11, but I think it's a mistake to
explicitly support this use case as anything other than UB.

>
> Thanks,
> Florian
>