Work around the memmove bug on Samsung device #4402

beeender · 2017-03-30T08:11:25Z

Zhuinden · 2017-03-30T08:13:14Z

If this works.... Celebration 😄

nhachicha · 2017-03-30T09:01:21Z

is this function hooking? we used similar approach in ReactNative https://github.com/realm/realm-js/blob/master/src/android/jsc_override.cpp#L60-L126

cmelchior

Left a few comments, but would rather have someone from #core reviewing this.

cmelchior · 2017-03-30T09:04:28Z

realm/realm-library/src/main/cpp/jni_util/hack.cpp

+    return (*s_wrap_memmove_ptr)(dest, src, n);
+}
+
+static void check_memmove()


Did this test come from anywhere? Then a link to that would be nice

it's from https://code.google.com/p/android/issues/detail?id=81692

cmelchior · 2017-03-30T09:12:47Z

realm/realm-library/src/main/cpp/io_realm_internal_SharedRealm.cpp

@@ -57,6 +59,8 @@ JNIEXPORT void JNICALL Java_io_realm_internal_SharedRealm_nativeInit(JNIEnv* env
 {
    TR_ENTER()

+    hack_init();


Shouldn't this be called in JNI_OnLoad or something? There is no guarantee that this code is the first native code being called?

UH, yes, JNI_OnLoad() would be a better place! But first need to make this patch work ...

I think using __attribute__((constructor)); is also an option, if I remember it runs when the .so is loaded https://github.com/realm/realm-js/blob/master/src/android/jsc_override.cpp#L38

__attribute__((constructor)); looks much better, but it seems to be a gcc extension, i am not quite sure if clang support it. I will check and replace the JNI_onLoad() if it supported by clang as well.

…o mc/bug/hack-memmove

beeender · 2017-03-30T10:36:43Z

@nhachicha no, the code you posted is really hacky 😁 It modifies the machine code in RAM during run time. This will just link to our wrapped version function during the linking time.

Zhuinden · 2017-03-30T11:11:35Z

realm/realm-library/src/main/cpp/jni_util/hack.cpp

+		s = (uint8_t*)src;
+		while (count--)
+			*tmp++ = *s++;
+	} else {


whitespace is weird here

There was a bug for memmove on some Samsung devices. The functions returns "dest + n" instead of "dest". This has been identified earlier by QT. See: https://bugreports.qt.io/browse/QTBUG-34984 The relevant android bug: https://code.google.com/p/android/issues/detail?id=81692 This fix try to test if the device have this issue first, if yes, switch to a own implementation of memmove. This fix works for most cases, but since the memmove bug existing in the system, there would be other lib/system call the buggy version of memmove which could still corrupt the memory -- which means there are still some possibilities that app gets crashed on those devices. The more hacky way is hooking into the process's code space and modify the function point to our own implementation which could solve the problem for other lib/system calls. But there is a risk that google might reject the apk for play store.

beeender · 2017-04-01T10:34:25Z

realm/realm-library/src/main/cpp/jni_util/bcopy.hpp

+namespace realm {
+namespace jni_util {
+
+void* my_bcopy(void *, const void *, std::size_t);


my_bcopy sounds strange... just __bcopy? Also, no need to have this in an independent header since it is not supposed to be used for any other places, just extern it from the hack.cpp?

kneth · 2017-04-01T10:58:51Z

@beeender I have asked our ops team for legal advice since we are including 3rd party code. Please do not merge before we have a clear understanding of the legal implications.

Zhuinden · 2017-04-04T08:11:39Z

@kneth any luck? This fix is super-duper important

cmelchior · 2017-04-04T08:12:48Z

@Zhuinden We just pinged our legal.
We want this released just as much as you 😄

dalinaum · 2017-04-04T08:21:12Z

Sumsung? Samsung? It seems a typo on this PR's title and commit logs.

dalinaum · 2017-04-04T08:39:37Z

realm/realm-library/src/main/cpp/jni_util/hack.cpp

+        s_wrap_memcpy_ptr =  &hacked_memmove;
+    }
+    else {
+        Log::i("memmove is not broken on this device - lucky you.");


Should it be logged? So many users would be lucky.

No this shouldn't be logged :)

Zhuinden · 2017-04-05T11:10:09Z

Shame that 3.1.0 did not have this, hoping for 3.1.1 😄

cmelchior · 2017-04-05T11:10:43Z

Yes, we wil make a patch release with this as soon as legal gives a 👍

beeender · 2017-04-05T13:22:00Z

need to benchmark with/without wrap memcpy

beeender · 2017-04-05T14:34:42Z

Wrap both memmove and memcpy:

wrap only memmove

no noticeable performance difference.

…o mc/bug/hack-memmove

Zhuinden · 2017-04-07T10:41:21Z

Switching to DRY libc11's implementations

I guess the legal team said you need one with a bit more permissive license

cmelchior

Only nitpicking comments 👍

cmelchior · 2017-04-07T10:44:46Z

realm/realm-library/src/main/cpp/jni_util/hack.cpp

+// See https://github.com/realm/realm-java/issues/3651#issuecomment-290290228
+// There is a bug in memmove for some Samsung devices which will return "dest-n" instead of dest.
+// The bug was originally found by QT, see https://bugreports.qt.io/browse/QTBUG-34984 .
+// To work around it, we use linker's wrap feature to use a pure C implementation of memmove if the device has the


cmelchior · 2017-04-07T10:45:50Z

realm/realm-library/src/main/cpp/jni_util/hack.cpp

+    size_t len = strlen(array);
+    void* ptr = __real_memmove(array + 1, array, len - 1);
+    if (ptr != array + 1 || strncmp(array, "FFooba", len) != 0) {
+        Log::e("memmove is broken on this device. Switching to the builtin implementation.");


builtin reads wrong to me here. What about Switching to a version provided by Realm ?

The underlying issue turned out to be a bug in `memmove` on some Android devices. It was addressed via realm/realm-java#4402.

Work around the memmove bug on sumsung device

27e9889

realm-ci assigned beeender Mar 30, 2017

stupid pointer mistake

0f8a402

cmelchior reviewed Mar 30, 2017

View reviewed changes

beeender and others added 5 commits March 30, 2017 17:35

3rd try, use kg's test and builtin fun

13c301c

Logging if memmove() isn't broken

1e72ba1

use __realm_memove for testing

f6b7f55

Merge branch 'mc/bug/hack-memmove' of github.com:realm/realm-java int…

aab63aa

…o mc/bug/hack-memmove

use memmove impl from kernel

5211cca

beeender force-pushed the mc/bug/hack-memmove branch from 4e69511 to 5211cca Compare March 30, 2017 09:55

Merge branch 'mc/bug/hack-memmove' of github.com:realm/realm-java int…

977f288

…o mc/bug/hack-memmove

Zhuinden reviewed Mar 30, 2017

View reviewed changes

Zhuinden mentioned this pull request Mar 30, 2017

RealmMigrationNeededException only on Android 4.2.2 and Samsung Galaxy S3 Mini #4306

Closed

beeender force-pushed the mc/bug/hack-memmove branch from 5211cca to dcae93b Compare March 30, 2017 12:05

beeender mentioned this pull request Mar 30, 2017

Samsung Tab 3 Lite 7 crashes on Realm 2.0.1 and 2.0.2 #3651

Closed

beeender added S:In Progress T-Bug-Crash labels Mar 30, 2017

beeender commented Apr 1, 2017

View reviewed changes

Zhuinden mentioned this pull request Apr 1, 2017

Upgrade to Realm 3.x Zhuinden/realm-book-example#8

Closed

dalinaum reviewed Apr 4, 2017

View reviewed changes

beeender changed the title ~~Work around the memmove bug on sumsung device~~ Work around the memmove bug on Samsung device Apr 5, 2017

beeender added 3 commits April 5, 2017 20:41

PR fix

17291ff

Merge remote-tracking branch 'origin/releases' into mc/bug/hack-memmove

b731047

Remove useless log

026827e

Only apply the hack to arm32

51e0c37

kneth approved these changes Apr 6, 2017

View reviewed changes

Kenneth Geisshirt added 3 commits April 7, 2017 11:30

Switching to DRY libc11's implementations

d50fb04

Merge branch 'mc/bug/hack-memmove' of github.com:realm/realm-java int…

29f2f52

…o mc/bug/hack-memmove

fixing compilation error

c23feab

cmelchior approved these changes Apr 7, 2017

View reviewed changes

beeender added S:Review and removed S:In Progress labels Apr 7, 2017

beeender merged commit 6ad1e13 into releases Apr 7, 2017

beeender removed the S:Review label Apr 7, 2017

beeender deleted the mc/bug/hack-memmove branch April 7, 2017 11:28

bdash added a commit to realm/realm-object-store that referenced this pull request Jun 26, 2017

Remove an outdated comment in create_metadata_tables.

95e253e

The underlying issue turned out to be a bug in `memmove` on some Android devices. It was addressed via realm/realm-java#4402.

kneth mentioned this pull request Nov 2, 2017

Crash on samsung realm/realm-js#1163

Closed

github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Work around the memmove bug on Samsung device #4402

Work around the memmove bug on Samsung device #4402

beeender commented Mar 30, 2017 •

edited

Zhuinden commented Mar 30, 2017

nhachicha commented Mar 30, 2017

cmelchior left a comment

cmelchior Mar 30, 2017

Zhuinden Mar 30, 2017

cmelchior Mar 30, 2017

beeender Mar 30, 2017

nhachicha Mar 30, 2017

beeender Apr 1, 2017 •

edited

beeender commented Mar 30, 2017

Zhuinden Mar 30, 2017

beeender Apr 1, 2017

kneth commented Apr 1, 2017

Zhuinden commented Apr 4, 2017

cmelchior commented Apr 4, 2017

dalinaum commented Apr 4, 2017 •

edited

dalinaum Apr 4, 2017 •

edited

cmelchior Apr 5, 2017

Zhuinden commented Apr 5, 2017

cmelchior commented Apr 5, 2017

beeender commented Apr 5, 2017

beeender commented Apr 5, 2017

Zhuinden commented Apr 7, 2017

cmelchior left a comment

cmelchior Apr 7, 2017

cmelchior Apr 7, 2017

Work around the memmove bug on Samsung device #4402

Work around the memmove bug on Samsung device #4402

Conversation

beeender commented Mar 30, 2017 • edited

Zhuinden commented Mar 30, 2017

nhachicha commented Mar 30, 2017

cmelchior left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beeender Apr 1, 2017 • edited

Choose a reason for hiding this comment

beeender commented Mar 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kneth commented Apr 1, 2017

Zhuinden commented Apr 4, 2017

cmelchior commented Apr 4, 2017

dalinaum commented Apr 4, 2017 • edited

dalinaum Apr 4, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zhuinden commented Apr 5, 2017

cmelchior commented Apr 5, 2017

beeender commented Apr 5, 2017

beeender commented Apr 5, 2017

Zhuinden commented Apr 7, 2017

cmelchior left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beeender commented Mar 30, 2017 •

edited

beeender Apr 1, 2017 •

edited

dalinaum commented Apr 4, 2017 •

edited

dalinaum Apr 4, 2017 •

edited