I have found the real culprit - it was actually in memcpy_backwards():
diff --git a/arch/x64/string.cc b/arch/x64/string.cc
index e31bc674..b3782fe7 100644
--- a/arch/x64/string.cc
+++ b/arch/x64/string.cc
@@ -400,7 +400,7 @@ void *memcpy_backwards(void *dst, const void *src,
size_t n)
if (!n--) {
return dst;
}
- *d-- = *s--;
+ *(--d) = *(--s);
}
long_backwards(d, s, n);
I will be sending a patch with an improved unit test later.
Waldek
On Friday, February 28, 2020 at 8:51:28 AM UTC-5, Waldek Kozaczuk wrote:
>
> I found the test that fails before my fix and succeeds with:
>
> static void test_memmove(size_t offset, size_t len)
> {
> printf("test_memmove: %ld, %ld\n", offset, len);
> char *buf10 = (char*)malloc(0x10000);
> for(int i = 0; i < 0x10000; i++)
> buf10[i] = i % 256;
> char a = buf10[offset + len];
> memmove(&buf10[offset], &buf10[0], len);
> assert(buf10[0]== buf10[offset]); // Verify that data got copied - 1st
> char
> assert(buf10[offset + len] == a); // Verfiy that next char after the
> target destination is still the same - LINE 35
> free(buf10);
> }
>
> test_memmove(381, 8);
> test_memmove(4, 2303);
> test_memmove(40, 2692);
> test_memmove(10, 2732);
>
> Cmdline: /tests/tst-memmove.so
> test_ala: 381, 8
> test_ala: 4, 2303
> test_ala: 40, 2692
> Assertion failed: buf10[offset + len] == a
> (/home/wkozaczuk/projects/osv/tests/tst-memmove.cc: test_memmove: 35)
>
> [backtrace]
> 0x000000004023b59e <__assert_fail+30>
> 0x0000100000006940 <???+26944>
> 0x0f0e0d0c0b0a0907 <???+185207047>
> Test tst-memmove.so FAILED
>
> So it looks like memcpy_backwards() overwrites some portion of memory it
> is not supposed to touch. I am still surprised how regular memcpy() seems
> to be handling overlapping area.
>
> On Thursday, February 27, 2020 at 11:58:29 PM UTC-5, Waldek Kozaczuk wrote:
>>
>> Adding this to tst-memmove.cc:
>>
>> char buf9[9] = "12344321";
>> memmove(&buf9[0], &buf9[1], 5);
>> pass_if(buf9, "23443321", 8);
>>
>>
>> still passes with current supposedly incorrect implementation. But
>> memcpy() in general is not expected to handle overlapping areas. So how
>> does it work?
>>
>> Either way something is wrong given that new implementation I proposed
>> fixes that YCMB redis crash.
>>
>> On Thursday, February 27, 2020 at 11:34:28 PM UTC-5, Waldek Kozaczuk
>> wrote:
>>>
>>> As I was investigating new issue -
>>> https://github.com/cloudius-systems/osv/issues/1074, I have finally
>>> realized we a have bug in our memmove() implementation -
>>> https://github.com/cloudius-systems/osv/blob/master/libc/string/memmove.c.
>>> We even have a unit test but apparently we missed some scenarios.
>>>
>>> Here is the code:
>>>
>>> void *memmove(void *dest, const void *src, size_t n)
>>> {
>>> char *d = dest;
>>> const char *s = src;
>>>
>>> if (d==s) return d;
>>> if (s+n <= d || d+n <= s) return memcpy(d, s, n);
>>>
>>> if (d<s) {
>>> return memcpy(d, s, n);
>>> } else {
>>> return memcpy_backwards(d, s, n);
>>> }
>>> }
>>>
>>> Let say we have following arguments:
>>> dest = 0x10
>>> src = 0x11
>>> n = 5
>>>
>>> The first 2 if would yield false. The last if would execute 1st branch
>>> and call memcpy which is wrong as the dest and src overlap, right?
>>>
>>> The correct code (probably needs to be optimized, can it?) should look
>>> something like this:
>>>
>>> void *memmove(void *dest, const void *src, size_t n)
>>> {
>>> char *d = dest;
>>> const char *s = src;
>>>
>>> if (d==s || n <= 0) return d;
>>> if (s+n <= d || d+n <= s) return memcpy(d, s, n);
>>>
>>> if (d<s) {
>>> while(n--) {
>>> *(d++) = *(s++);
>>> }
>>> } else {
>>> for (size_t i = n; i > 0; i--) {
>>> *(d + i - 1) = *(s + i - 1);
>>> }
>>> }
>>> return dest;
>>> }
>>>
>>> With this change, the reported crash goes away.
>>>
>>> Waldek
>>>
>>>
>>>
--
You received this message because you are subscribed to the Google Groups "OSv
Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to osv-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/osv-dev/cc357887-ecbb-4c87-ad00-45008ea5a3cb%40googlegroups.com.
