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

MsgPack spec v5 support #36

Closed
mattheworiordan opened this issue Oct 21, 2014 · 11 comments
Closed

MsgPack spec v5 support #36

mattheworiordan opened this issue Oct 21, 2014 · 11 comments

Comments

@mattheworiordan
Copy link

Looking through the code for this library, it appears that v5 of the MsgPack protocol is not supported. See https://github.com/msgpack/msgpack/blob/master/spec.md, with some of the big changes at the time being that RAW is now split into str & bin 8/16/32 types. Looking at the unpacker, I don't see any mention of these new types.

What is the status on this, and does anyone currently have plans to add support for the MsgPack protocol v5?

@docteurklein
Copy link

i'd very much like to see v5 supported as well.

@xiaoxiangdu
Copy link

I want V5 too, Any body can give a plan?

@mattheworiordan
Copy link
Author

@dawnlucky and @docteurklein I've been waiting a good old while, looks like we might need to implement ourselves.

@xiaoxiangdu
Copy link

Hey guys, I asked @laruence from weibo, he said he will implement this feature one or two months later after release of PHP7 RC.

@laruence
Copy link
Member

laruence commented Apr 2, 2015

yeah, I am kind of busy recently.. sorry for that, and if anyone could implement this, it will be appreciated..

@Sean-Der
Copy link
Member

Hey everyone, I am working on PHPNG support in PR #48

I would love to get v5 support into that branch! I will not be working on until I get all the tests passing in that branch (2 weeks I am thinking! Depending on my work schedule)

If someone wants to make some basic test cases, I would really appreciate it! This also makes sure than when I say 'it is done!' the thing you want to serialize/deserialize just works.

@Sean-Der
Copy link
Member

Hey @mattheworiordan

Deserialization support for the bin format family has been added and str8 serialization/deserialization is supported now.

The only thing left I think is the 'compatibility mode' that would disable str8 and #49 has already been filed to address that.

@mattheworiordan
Copy link
Author

@Sean-Der nice work! Once #49 is resolved, then we will implement in our client libraries, see ably/ably-php#14. Thanks again.

@laruence
Copy link
Member

@Sean-Der, actually I am thinking a problem of security.. we have no way to detect an input is legal. that means, for string unserialize we do "ZVAL_STRING(zv, (char*)bin, size);" , so if an attacker give us a huge size, we may result in segfault(invalid read at least)..

maybe we should add some protection, like the size of a str/bin can not be greater than the whole serialized string length...

What do you think?

@Sean-Der
Copy link
Member

@laruence I think that is a excellent idea! I did some snooping in the Python implementation and found this want me to open a PR that adds default limits for bin/str/array/map default limits that can also be set by the user?

@laruence
Copy link
Member

hmm, add a limit is not the best solution... for bin/raw, if we found the len is greater than the rest serilazlied string, then -> error.

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

No branches or pull requests

5 participants