mrousavy / react-native-mmkv

āš”ļø The fastest key/value storage for React Native. ~30x faster than AsyncStorage!
https://mrousavy.com
MIT License
5.87k stars 253 forks source link

Encrypted instance cannot store strings over 253 characters #707

Closed tcK1 closed 3 weeks ago

tcK1 commented 1 month ago

Hey! First of all, thanks for the amazing package.

I'm currently exploring the package within the New Architecture and found the following behavior: When setting strings with over 253 characters, the package does not return the respective value.

For example, here are the values that work and don't work:

Works:

[Qtdiwf-DwGR8#?9rkScC[r&Zzf*5t[6whBz[gaS&9Cd5#JQL*-wQ:8FdSM!0;jvjrA,AkUuKfdxdnEt-e_&gbn9.;&TRZ]gy8[}gx*@1.:wJ;$zT9KiQdLTCMn(Qy!;xebRY:CtjdQynyc.=qY9.,(vT[B1py@_aVyLL?Sji_?0JM(:*/DhrMJ[N!&wS69i9xA,Yjmb[A+u&eN}*N/?&(6?4x_@a{yK6!kGj1H{7WPwG9H9pd?@7,*_yJJ[*

Doesn't work (Added an + in the end):

[Qtdiwf-DwGR8#?9rkScC[r&Zzf*5t[6whBz[gaS&9Cd5#JQL*-wQ:8FdSM!0;jvjrA,AkUuKfdxdnEt-e_&gbn9.;&TRZ]gy8[}gx*@1.:wJ;$zT9KiQdLTCMn(Qy!;xebRY:CtjdQynyc.=qY9.,(vT[B1py@_aVyLL?Sji_?0JM(:*/DhrMJ[N!&wS69i9xA,Yjmb[A+u&eN}*N/?&(6?4x_@a{yK6!kGj1H{7WPwG9H9pd?@7,*_yJJ[*+

I've made a small reproduction repo, but here's the following steps:

It seems that this happens both on Android and iOS.

Versions: react-native: 0.74.3 react-native-mmkv: 3.0.0-beta.7

maintenance-hans[bot] commented 1 month ago

Guten Tag, Hans here.

[!NOTE] New features, bugfixes, updates and other improvements are all handled mostly by @mrousavy in his free time. To support @mrousavy, please consider šŸ’– sponsoring him on GitHub šŸ’–. Sponsored issues will be prioritized.

mrousavy commented 1 month ago

Hey!

Hey! First of all, thanks for the amazing package.

Thank you! :)

When setting strings with over 253 characters, the package does not return the respective value

Hm, I just tested it with this code:

try {
  console.log(`setting...`);
  storage.set(
    'dummy_key',
    '[Qtdiwf-DwGR8#?9rkScC[r&Zzf*5t[6whBz[gaS&9Cd5#JQL*-wQ:8FdSM!0;jvjrA,AkUuKfdxdnEt-e_&gbn9.;&TRZ]gy8[}gx*@1.:wJ;$zT9KiQdLTCMn(Qy!;xebRY:CtjdQynyc.=qY9.,(vT[B1py@_aVyLL?Sji_?0JM(:*/DhrMJ[N!&wS69i9xA,Yjmb[A+u&eN}*N/?&(6?4x_@a{yK6!kGj1H{7WPwG9H9pd?@7,*_yJJ[*+'
  );
  console.log(`set successfully!`);
  const val = storage.getString('dummy_key')
  console.log(
    `dummy_key length: ${val?.length}; value: ${val}`
  );
} catch (e) {
  console.log(`failed! ${e}`);
}

..and this works perfectly fine:

 (NOBRIDGE) LOG  setting...
 (NOBRIDGE) LOG  set successfully!
 (NOBRIDGE) LOG  dummy_key length: 254; value: [Qtdiwf-DwGR8#?9rkScC[r&Zzf*5t[6whBz[gaS&9Cd5#JQL*-wQ:8FdSM!0;jvjrA,AkUuKfdxdnEt-e_&gbn9.;&TRZ]gy8[}gx*@1.:wJ;$zT9KiQdLTCMn(Qy!;xebRY:CtjdQynyc.=qY9.,(vT[B1py@_aVyLL?Sji_?0JM(:*/DhrMJ[N!&wS69i9xA,Yjmb[A+u&eN}*N/?&(6?4x_@a{yK6!kGj1H{7WPwG9H9pd?@7,*_yJJ[*+
mrousavy commented 1 month ago

I also just tried to set a 10.000 character string, which works perfectly fine.

Code:

const storage = new MMKV();

function randomChar(): string {
  const index = Math.floor(Math.random() * 10);
  return 'abcdefghij'[index] as string;
}

const KEY = 'dummy_value';
for (let i = 0; i < 1000; i++) {
  const str = [...Array(i)].map(() => randomChar()).join('');
  console.log(`Setting ${KEY} to a ${i} character string.. (${str})`);
  storage.set(KEY, str);
  const actual = storage.getString(KEY);
  console.log(
    `Returned: ${actual?.length} vs ${str.length}, is equal: ${str === actual}`
  );
}

Result:

[...998 previous lines]
 (NOBRIDGE) LOG  Setting dummy_value to a 999 character string.. (hiidjiifdabdibbheibafhjjbdhjgdcbcbchbgfacahdjfeehaafehgahgbaghaijdafbedeebbidgbjdbhceijahbfhbdcebbgchdjdbgahcjdebbaibghgcedcgbeifeahejggaeahbjchdbjeedjidbadhfdhfjcffaegbgbfijfhbgebcibcchdefggfgibffdagijdcbajdhgceceeijcaebcbigdcaicehjiajdgaigacfdhcbdedfabcddifhgacbhdjdbicbiicifgcagefabjbijgehacibahgfjjcdiiiifcbcjeddfijajhcibdijacgdebiffccggeebabadfebggiaeiiejjbdicedbfjjgajffiiadchgjhgagicefeiefbbeahgdiiibihiciegheaghddecfibdbcbjbeeaiheajjdhejjbjecdfgcicejdcbicbehddccdccajeaafjagbbfgabgebadceabagdjfdibbeabeejbgbeajafehadegdefiegeeggjdhfjcifjjbhdifiddjdbfddgcaefgfgiaggcddhabaeeajaaidicfaaiabeibbjhgafffjeaahbjiabjbhejhhhjheggjgdbejccggabfhbgicbehbcgdejidbibgchccgceffafhhdbgdiheegaiicchgbjhdcgadhceciijdiabgjbeebfcegbahjhifhgbhfebbbdfadaajjdbiehhagebccbhdcdffegfbdcheidhcaecffefghafdcajgbigdaiadhdaecaghifhgbidhecceeaeafgcahafdjfcbjchijfbihfchjidchbhfefjfifcgjjajifbcadjjicgehagieeajbjcbgfjecjfajaiaaiiicbcehdiaicbfjeggijjcfbcbefhbghajeehijbcgacegchhfbhjccfbjhgahcbejaibhbeeahgjdcfeciaifggdaefgb)
 (NOBRIDGE) LOG  Returned: 999 vs 999, is equal: true
tcK1 commented 1 month ago

Hey @mrousavy, thanks for responding!

Seems like the problem only happens with encrypted storage. I'm sorry, I should've mentioned it in the issue description.

For example:

export const encryptedStorage = new MMKV({
  id: 'instance-id',
  encryptionKey: 'secret-pwd',
});
mrousavy commented 1 month ago

Tested on an iPhone 15 Pro Max simulator, in MMKV Example app on latest main.

mrousavy commented 1 month ago

Ah yes you are right. With my latest for loop test this works up to 253, but breaks at 254.

 (NOBRIDGE) LOG  Setting dummy_value to a 253 character string.. (ahjidahhfcdfheaiagafbfgefieeafbjjjdcdfegfbideahfdgdhijgjcecdjchgeiejaihjhfifhbffibcdeadgdjhddigbidgjbjbchigaahchbedgigaiagbdcbcejcggfjcjifjbffjjadidhbgdcjhjheggifbgihdgjfiahjiefbafjfebdcfcjibhacgdeiagbfggajbgaihfdecfjdhjbcdfgbgbjhdghdejjhjgfebddgdhighdc)
 (NOBRIDGE) LOG  Returned: 253 vs 253, is equal: true, result: ahjidahhfcdfheaiagafbfgefieeafbjjjdcdfegfbideahfdgdhijgjcecdjchgeiejaihjhfifhbffibcdeadgdjhddigbidgjbjbchigaahchbedgigaiagbdcbcejcggfjcjifjbffjjadidhbgdcjhjheggifbgihdgjfiahjiefbafjfebdcfcjibhacgdeiagbfggajbgaihfdecfjdhjbcdfgbgbjhdghdejjhjgfebddgdhighdc
^ worked

 (NOBRIDGE) LOG  Setting dummy_value to a 254 character string.. (ibjbaefffbeehhhhhbfcjjhebcchdbfgbiijbjjbcbcgeediahgagcigegahciiicfgebicejhgbdebiaaacaegjfdcicbfhjgacjcdibhggajhgdjjhiifjefgcefjbcjagejhbbejfgegjhgcbhhibheccdaffghhbbdafheeaajhbbchghjgaidefebgecicbceabaihgfddddeebefigidbdfgdhagjcaecddebebjgbdijfgifigejgge)
 (NOBRIDGE) LOG  Returned: 2 vs 254, is equal: false, result: M
^ failed

But I think this is a Tencent/MMKV issue (or size limitation), I don't think I am doing anything wrong in set: https://github.com/mrousavy/react-native-mmkv/blob/b99c51ee0894e149c15eae8934ed48d4ab2237d6/package/cpp/MmkvHostObject.cpp#L107-L110

..and successful is even true, so it doesn't throw an error here.

Would be great if you can create an issue over at https://github.com/Tencent/MMKV šŸ‘

tcK1 commented 1 month ago

Yes! Ofc, I will open the issue there as well!

Just to clarify, this behavior only happens with the 3.0.0 release. Using the 2.12.2 (and thus with the New Architecture disabled), it works as expected:

 BUNDLE  ./index.js 

 LOG  Value: [Qtdiwf-DwGR8#?9rkScC[r&Zzf*5t[6whBz[gaS&9Cd5#JQL*-wQ:8FdSM!0;jvjrA,AkUuKfdxdnEt-e_&gbn9.;&TRZ]gy8[}gx*@1.:wJ;$zT9KiQdLTCMn(Qy!;xebRY:CtjdQynyc.=qY9.,(vT[B1py@_aVyLL?Sji_?0JM(:*/DhrMJ[N!&wS69i9xA,Yjmb[A+u&eN}*N/?&(6?4x_@a{yK6!kGj1H{7WPwG9H9pd?@7,*_yJJ[*+[Qtdiwf-DwGR8#?9rkScC[r&Zzf*5t[6whBz[gaS&9Cd5#JQL*-wQ:8FdSM!0;jvjrA,AkUuKfdxdnEt-e_&gbn9.;&TRZ]gy8[}gx*@1.:wJ;$zT9KiQdLTCMn(Qy!;xebRY:CtjdQynyc.=qY9.,(vT[B1py@_aVyLL?Sji_?0JM(:*/DhrMJ[N!&wS69i9xA,Yjmb[A+u&eN}*N/?&(6?4x_@a{yK6!kGj1H{7WPwG9H9pd?@7,*_yJJ[*+
 LOG  Running "Mmkvnewarch" with {"rootTag":11,"initialProps":{"concurrentRoot":false}}

Did we update the MMKV underlying version? Maybe I can add this information to the issue over there!

mrousavy commented 1 month ago

Interesting - so it's either a recent MMKV upgrade, or maybe the fact that I now use the C++ version of MMKV instead of the Objective-C version.

mrousavy commented 1 month ago

Actually on Android I always used the C++ (POSIX) version, on v2 and on v3 - so if the behaviour is the same there (v2 works, v3 breaks), then it's def a recent upgrade.

tcK1 commented 1 month ago

I've just tested with 2.12.2 on Android and the problem also happens! Seems like this is related to the C++ version then. Thanks for the direction, I will open the issue on their repo!

tcK1 commented 1 month ago

Opened https://github.com/Tencent/MMKV/issues/1361. šŸ‘šŸ¼

tcK1 commented 1 month ago

The issue seems to be fixed at https://github.com/Tencent/MMKV/tree/dev!

I patched the MMKV/Core folder locally with the content on the branch and it seems like the problem was solved.

Thanks for the help investigating the issue and I will wait for a new version (both of MMKV, I guess, and here).

It would be nice if a patch version for v2 (which does not need the New Architecture) was released as well, as it would fix the issue on Android devices.

If we can point to the dev branch, I could help by opening a PR!

Again, thanks a lot! šŸ˜Š

mendozammatias commented 4 weeks ago

Hey! I'm using v2 since I can't enable the new architecture in the project I'm working on. Is there any way to publish an update for v2 pointing to the fixed MMKV branch? More than happy to collaborate in any possible way.

mrousavy commented 3 weeks ago

Fixed in react-native-mmkv 3.0.0!