Skip to content
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

MMKV.mmkvWithID crash #60

Closed
rednels opened this issue Sep 30, 2018 · 20 comments
Closed

MMKV.mmkvWithID crash #60

rednels opened this issue Sep 30, 2018 · 20 comments

Comments

@rednels
Copy link

rednels commented Sep 30, 2018

MMKV.mmkvWithID("test/hello2").encode("hello", "world")

id中使用/会导致崩溃

@lingol
Copy link
Collaborator

lingol commented Sep 30, 2018

Well, MMKV uses id as filename. For the time being, just don't do this.

@rednels
Copy link
Author

rednels commented Sep 30, 2018

Why not Hash Code or MD5

@lingol
Copy link
Collaborator

lingol commented Sep 30, 2018

Why not Hash Code or MD5

It's already shipped. Now is too late for that. That's why.

@ysbing
Copy link

ysbing commented Sep 30, 2018

Why not Hash Code or MD5

It's already shipped. Now is too late for that. That's why.

Is it possible to encode only special characters?

@lingol
Copy link
Collaborator

lingol commented Sep 30, 2018

Is it possible to encode only special characters?

Or just throw exception on bad id?

Encoding special char is always complicated. Say you replaces / with _, now how do you handle normal _? With __? Now don't forget there may already be some id that has an _, how do you handle old id?

@zhongwuzw
Copy link
Contributor

Is this related to Android? I don't familiar with it, butI think we can find the issue character and just encode it, the way like encodeURI which transform certain characters using escape sequences.

@lingol
Copy link
Collaborator

lingol commented Oct 3, 2018

Is this related to Android? I don't familiar with it, butI think we can find the issue character and just encode it, the way like encodeURI which transform certain characters using escape sequences.

No. It has nothing to do with Android. All those ecodeXXX has the same problem I described before: they don't support backward compatibility.

@lingol
Copy link
Collaborator

lingol commented Oct 9, 2018

Is it possible to encode only special characters?

Or just throw exception on bad id?

Fixed with commit.

@lingol lingol closed this as completed Oct 9, 2018
@ysbing
Copy link

ysbing commented Oct 12, 2018

Other char?
qwe

@lingol
Copy link
Collaborator

lingol commented Oct 12, 2018

Other char?
qwe

Hmm...

@zhongwuzw
Copy link
Contributor

zhongwuzw commented Oct 12, 2018

@lingol

No. It has nothing to do with Android. All those ecodeXXX has the same problem I described before: they don't support backward compatibility.

No backward compatible? I think it compatible, previously it would crash, after encode specific char, it works.

@lingol
Copy link
Collaborator

lingol commented Oct 12, 2018

No backward compatible? I think it compatible, previously it would crash, after encode specific char, it works.

Encoding special char is always complicated. Say you replaces / with _, now how do you handle normal _? With __? Now don't forget there may already be some id that has an _, how do you handle old id?

Had I not been clear? Those encodeXXX algorithms, no matter which char(s) they choose to escape, are choosing valid chars to escape with. And how do they handle these valid chars themself? They do double escape. That's where the No backward compatible happens.

@zhongwuzw
Copy link
Contributor

@lingol First, scan the mmkvID, if it exist special character (which leads to crash), encode it and using a separate sub-directory in directory mmkv to store these files which have special characters. If it not have special character, keep the regular way.

@lingol
Copy link
Collaborator

lingol commented Oct 12, 2018

@zhongwuzw

Say we replace / with _.

Given test/hello2, we get test_hello2, right? Everything goes well. Except if there is some one already create a MMKV instance with test_hello2.

Now tell me, how do you handle this collision?

@lingol lingol reopened this Oct 12, 2018
@zhongwuzw
Copy link
Contributor

@lingol Emm, maybe you don't get what I point 😂 . I'll explain further, as your example, if given test/hello2, we find it contains special character /, so we encode it to test_hello2, and besides this, we add a separate subdirectory to store the file, so file would be stored in mmkv/special-character/ directory (PS. for id which not contains special character, we still store the files in mmkv/ directory). So no collision would be happen.

@ajinwu
Copy link

ajinwu commented Oct 12, 2018 via email

@lingol
Copy link
Collaborator

lingol commented Oct 13, 2018

@zhongwuzw That might just work. Would you like to write a pr?

@zhongwuzw
Copy link
Contributor

@lingol Yeah, I can, but firstly, I need to confirm something because I'm not familiar Android, the reason which causes crash is the file cannot created? because intermediate directories not exist if id contains /? If it is, I think we only need to modify implementation mappedKVPathWithID and crcPathWithID, and judge wether contains /, if existed, encode it and create a separate sub-directory, am I right?

@lingol
Copy link
Collaborator

lingol commented Oct 13, 2018

Other char?
qwe

@zhongwuzw Not just /.

@lingol
Copy link
Collaborator

lingol commented Nov 14, 2018

Release with v1.0.13.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants