Fix error that caused a segfault when users registered custom Sync logger functions #4156

fronck · 2021-12-16T20:06:04Z

What, How & Why?

This closes #4121

☑️ ToDos

📝 Changelog entry
📝 Compatibility label is updated or copied from previous entry
🚦 Tests
📱 Check the React Native/other sample apps work if necessary
📝 Public documentation PR created or is not necessary
💥 Breaking label has been applied or is not necessary

If this PR adds or changes public API's:

typescript definitions file is updated
jsdoc files updated
Chrome debug API is updated if API is available on React Native

…o ff/custom-sync-logger

kraenhansen

Great job. Was it basically that the function provided by the user was not owned by SyncLoggerDelegator and was therefore free to be garbage collected, eventually leading to a use after free of the callback?

kneth

Awesome ❤️

kneth · 2021-12-17T09:06:26Z

tests/js/session-tests.js

+    const realm = await Realm.open(config);
+    realm.close();
+
+    TestCase.assertTrue(loggerInvoked, "Custom logger should have been invoked");


I am a bit worry for a race condition here but maybe Realm.open() takes long enough.

tests/js/session-tests.js

tomduncalf · 2021-12-17T09:51:09Z

Great job. Was it basically that the function provided by the user was not owned by SyncLoggerDelegator and was therefore free to be garbage collected, eventually leading to a use after free of the callback?

My understanding is that the show_logs std::function was going out of scope at the end of the set_sync_logger method and as nothing else had taken ownership of it, it was being destructed immediately at that time... so by the time something tried to log and make_logger was called, it no longer existed. Franck may be able to correct any misunderstandings I have though :)

In the old implementation, there was a SyncLoggerDelegatorFactory class which retained ownership of the function.

tomduncalf · 2021-12-17T10:11:19Z

tests/js/session-tests.js

+    let loggerInvoked = false;
+    Realm.App.Sync.setLogLevel(app, "info"); // "all", "trace", "debug", "detail", "info", "warn", "error", "fatal", "off"
+    Realm.App.Sync.setLogger(app, (level, message) => (loggerInvoked = true));


I guess it might not hurt to test that level and message are passed through correctly?

Suggested change

let loggerInvoked = false;

Realm.App.Sync.setLogLevel(app, "info"); // "all", "trace", "debug", "detail", "info", "warn", "error", "fatal", "off"

Realm.App.Sync.setLogger(app, (level, message) => (loggerInvoked = true));

let loggedLevel, loggedMessage;

Realm.App.Sync.setLogLevel(app, "info"); // "all", "trace", "debug", "detail", "info", "warn", "error", "fatal", "off"

Realm.App.Sync.setLogger(app, (level, message) => {

if (!loggedLevel) loggedLevel = level;

if (!loggedMessage) loggedMessage = message;

});

// ...

TestCase.assertEqual(loggedLevel, 123);

TestCase.assertEqual(loggedMessage, "whatever the first message is");

Or is the first message not predictable?

It is as long as the sync subsystem doesn't change what it returns, or the sync config structure doesn't change. It'd be weird if the test failed because of that, but I'll see if I can parse out from the lower levels how the message is constructed.

I wouldn't worry too much if it's awkward :)

fronck · 2021-12-17T11:58:11Z

Great job. Was it basically that the function provided by the user was not owned by SyncLoggerDelegator and was therefore free to be garbage collected, eventually leading to a use after free of the callback?

My understanding is that the show_logs std::function was going out of scope at the end of the set_sync_logger method and as nothing else had taken ownership of it, it was being destructed immediately at that time... so by the time something tried to log and make_logger was called, it no longer existed. Franck may be able to correct any misunderstandings I have though :)

In the old implementation, there was a SyncLoggerDelegatorFactory class which retained ownership of the function.

Yes, basically we were passing log_fn around by reference, so when it went out of scope, we left a dangling pointer to a function that wasn't safe to invoke.

kraenhansen

I have a minor comment here - feel free to integrate the suggestion or merge at will.

kraenhansen · 2021-12-17T13:20:27Z

tests/js/session-tests.js

+    });
+
+    const realm = await Realm.open(config);
+    const loggerInvoked = await promisedLog;


There's no real meaning in returning and asserting true. If it doesn't happen the promise won't resolve and the test will simply time out.

Suggested change

const loggerInvoked = await promisedLog;

await promisedLog;

kraenhansen · 2021-12-17T13:22:02Z

tests/js/session-tests.js

+    const realm = await Realm.open(config);
+    const loggerInvoked = await promisedLog;
+    realm.close();
+    TestCase.assertTrue(loggerInvoked, `Custom logger should have registered an '${logLevelStr}' message`);


Suggested change

TestCase.assertTrue(loggerInvoked, `Custom logger should have registered an '${logLevelStr}' message`);

kraenhansen · 2021-12-17T13:23:22Z

tests/js/session-tests.js

+      Realm.App.Sync.setLogger(app, (level, message) => {
+        if (level == logLevelNum && message.includes("Connection") && message.includes("Session")) {
+          // we should, at some point, receive a log message that looks like
+          // Connection[1]: Session[1]: client_reset_config = false, Realm exists = true, client reset = false


If you want the test to rely on this message to eventually be logged, you could resolve conditionally on the message?

Franck Franck added 5 commits December 16, 2021 21:03

printf debugging of issue

ed04671

SyncLoggerDelegate now takes over ownership of the logging function

9c0b403

Added test for custom Sync session loggers

a8c06be

Updated changelog

f6576cb

Removed unnecessary comment lines

bc13b27

cla-bot bot added the cla: yes label Dec 16, 2021

fronck requested review from kneth and tomduncalf December 16, 2021 20:06

Franck Franck added 4 commits December 16, 2021 21:10

Fixed linter errors

8bc727e

Clean up changelog

ab83314

Removed unnecessary #include

b8a411c

Merge branch 'ff/custom-sync-logger' of github.com:realm/realm-js int…

83ffa3f

…o ff/custom-sync-logger

kraenhansen approved these changes Dec 16, 2021

View reviewed changes

kneth approved these changes Dec 17, 2021

View reviewed changes

kraenhansen reviewed Dec 17, 2021

View reviewed changes

tests/js/session-tests.js Outdated Show resolved Hide resolved

tomduncalf reviewed Dec 17, 2021

View reviewed changes

tomduncalf approved these changes Dec 17, 2021

View reviewed changes

Franck Franck added 2 commits December 17, 2021 13:58

Adopted requests from code review

2e66bed

Merge branch 'master' into ff/custom-sync-logger

51247b9

fronck requested review from tomduncalf and kraenhansen December 17, 2021 13:05

kraenhansen approved these changes Dec 20, 2021

View reviewed changes

Fixes requested in code review

7230921

fronck merged commit 01e3437 into master Dec 20, 2021

fronck deleted the ff/custom-sync-logger branch December 20, 2021 13:32

fronck mentioned this pull request Dec 20, 2021

Segmentation fault when performing await realm.open(config) #4121

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

Fix error that caused a segfault when users registered custom Sync logger functions #4156

Fix error that caused a segfault when users registered custom Sync logger functions #4156

fronck commented Dec 16, 2021

kraenhansen left a comment

kneth left a comment

kneth Dec 17, 2021

tomduncalf commented Dec 17, 2021

tomduncalf Dec 17, 2021

fronck Dec 17, 2021

tomduncalf Dec 17, 2021

fronck commented Dec 17, 2021

kraenhansen left a comment

kraenhansen Dec 17, 2021

kraenhansen Dec 17, 2021

kraenhansen Dec 17, 2021

Fix error that caused a segfault when users registered custom Sync logger functions #4156

Fix error that caused a segfault when users registered custom Sync logger functions #4156

Conversation

fronck commented Dec 16, 2021

What, How & Why?

☑️ ToDos

kraenhansen left a comment

Choose a reason for hiding this comment

kneth left a comment

Choose a reason for hiding this comment

kneth Dec 17, 2021

Choose a reason for hiding this comment

tomduncalf commented Dec 17, 2021

tomduncalf Dec 17, 2021

Choose a reason for hiding this comment

fronck Dec 17, 2021

Choose a reason for hiding this comment

tomduncalf Dec 17, 2021

Choose a reason for hiding this comment

fronck commented Dec 17, 2021

kraenhansen left a comment

Choose a reason for hiding this comment

kraenhansen Dec 17, 2021

Choose a reason for hiding this comment

kraenhansen Dec 17, 2021

Choose a reason for hiding this comment

kraenhansen Dec 17, 2021

Choose a reason for hiding this comment