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/3941871c-afc9-41b2-88c7-6f4bc9123b83%40googlegroups.com.
