New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix memory corruption in `writeCopyTo`. #2020

Merged
merged 1 commit into from Sep 13, 2018

Conversation

Projects
None yet
2 participants
@mandrigin
Copy link
Contributor

mandrigin commented Sep 12, 2018

What, How & Why?

We found an issue that it isn't possible to open a realm database if it was created as a copy using writeCopyTo and providing an encryption key.
The error looked like that:

1) RealmTests : testWriteCopyTo
   Error: Unable to open a realm at path '/Users/mandrigin/golang/src/github.com/realm/realm-js/tests/default.realm.copy.realm': Invalid mnemonic. top_ref[0]: 528EB59E00000001, top_ref[1]: E008049A66C646D4, mnemonic: D1 77 E4 5C, fmt[0]: 64, fmt[1]: 63, flags: F4.
       at Error (native)

After investigation, we noticed a memory corruption in the key, when it was sent to write_copy.

writeCopyTo / key data is: 
[0]=31 [1]=31 [2]=31 [3]=31 [4]=31 [5]=31 [6]=31 [7]=31 [8]=31 [9]=31 [10]=31 [11]=31 [12]=31 [13]=31 [14]=31 [15]=31 [16]=31 [17]=31 [18]=31 [19]=31 [20]=31 [21]=31 [22]=31 [23]=31 [24]=31 [25]=31 [26]=31 [27]=31 [28]=31 [29]=31 [30]=31 [31]=31 [32]=31 [33]=31 [34]=31 [35]=31 [36]=31 [37]=31 [38]=31 [39]=31 [40]=31 [41]=31 [42]=31 [43]=31 [44]=31 [45]=31 [46]=31 [47]=31 [48]=31 [49]=31 [50]=31 [51]=31 [52]=31 [53]=31 [54]=31 [55]=31 [56]=31 [57]=31 [58]=31 [59]=31 [60]=31 [61]=31 [62]=31 [63]=31

ERROR: Connection[1]: Failed to connect to endpoint '::1:9080': Connection refused
write_copy / key data is: [0]=ffffffad [1]=ffffffbe [2]=ffffffab [3]=fffffff9 [4]=68 [5]=6a [6]=ffffffdd [7]=ffffffba [8]=ffffff80 [9]=ffffff82 [10]=07 [11]=00 [12]=00 [13]=60 [14]=00 [15]=00 [16]=31 [17]=31 [18]=31 [19]=31 [20]=31 [21]=31 [22]=31 [23]=31 [24]=31 [25]=31 [26]=31 [27]=31 [28]=31 [29]=31 [30]=31 [31]=31 [32]=31 [33]=31 [34]=31 [35]=31 [36]=31 [37]=31 [38]=31 [39]=31 [40]=31 [41]=31 [42]=31 [43]=31 [44]=31 [45]=31 [46]=31 [47]=31 [48]=31 [49]=31 [50]=31 [51]=31 [52]=31 [53]=31 [54]=31 [55]=31 [56]=31 [57]=31 [58]=31 [59]=31 [60]=31 [61]=31 [62]=31 [63]=31

Gotcha: this was only reproducible on iOS 10+ and Android, tests on macos always passed. Probably iOS 8 and 9 and macos have more relaxed memory optimizations.

After a closer look, it seems like this code was responsible for that (my comments):

    BinaryData key;
    
    if (args.count == 2) {
        ValueType key_value = args[1];
        if (!Value::is_binary(ctx, key_value)) {
            throw std::runtime_error("Encryption key for 'writeCopyTo' must be a Binary.");
        }
        auto key_data = Value::validated_to_binary(ctx, key_value); // we create a local variable here
        key = { static_cast<const char *>(key_data.data()), key_data.size() }; // it's a pointer to the `key_data`
     } // here, `key_data` is freed and it's memory is up for grabs

     realm->write_copy(path, key); // key.data() points to a unused memory that can be overriden
}

I restructured the code a little bit and this bug doesn't appear anymore.

Steps to verify the problem

  1. Override tests/js/realm-tests.js with the version from this PR (required to work well on a mobile device).
  2. Tweak scripts/test.sh to run on a modern iOS (I hardcoded IOS_RUNTIME='com.apple.CoreSimulator.SimRuntime.iOS-11-0' locally)
  3. Remove and update dependencies of the react-test-app:
    pushd tests/react-test-app && rm -rf node_modules && npm install && popd
  4. scripts/test.sh react-tests

Expected: testWriteCopyTo fails with something like Invalid mnemonic. top_ref[0]: 528EB59E00000001, top_ref[1]: E008049A66C646D4, mnemonic: D1 77 E4 5C, fmt[0]: 64, fmt[1]: 63, flags: F4.

Steps to verify the solution

  1. Tweak scripts/test.sh to run on a modern iOS (I hardcoded IOS_RUNTIME='com.apple.CoreSimulator.SimRuntime.iOS-11-0' locally)
  2. Remove and update dependencies of the react-test-app:
    pushd tests/react-test-app && rm -rf node_modules && npm install && popd
  3. scripts/test.sh react-tests

Expected:testWriteCopyTo should pass successfully.

☑️ ToDos

  • 🚦 Tests

@mandrigin mandrigin force-pushed the mandrigin:bugfix/write-copy-to branch 3 times, most recently from badc1cb to 436734a Sep 12, 2018

@kneth

This comment has been minimized.

Copy link
Contributor

kneth commented Sep 13, 2018

@mandrigin Thanks for the fix. It looks good to me. I believe that you have fixed #1748 for us! I remember that writeCopyTo had some problems on some platforms when it was introduced but I didn't have the time to fix it - and created #1748 to remind me to come back to it.

Can you update CHANGELOG.md? Just something like "Fixed a memory corruption in writeCopyTo ".

@kneth kneth added the T:Bug label Sep 13, 2018

@kneth
Copy link
Contributor

kneth left a comment

👍 for the code. Waiting for an update of CHANGELOG.md.

@mandrigin mandrigin force-pushed the mandrigin:bugfix/write-copy-to branch from 436734a to dd7b76f Sep 13, 2018

@mandrigin

This comment has been minimized.

Copy link
Contributor Author

mandrigin commented Sep 13, 2018

@kneth cool, happy to help!
Updated CHANGELOG.md.

@kneth kneth merged commit 67dccf1 into realm:master Sep 13, 2018

@mandrigin

This comment has been minimized.

Copy link
Contributor Author

mandrigin commented Sep 13, 2018

@kneth yay! kudos for such swift responses! 🎉 top-notch contributor experience 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment