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

Fix TCP buffer parsing to find JSON statements to correctly #15591

Merged
merged 1 commit into from Mar 4, 2019

Conversation

Projects
None yet
5 participants
@DaveTBlake
Copy link
Member

commented Feb 23, 2019

Fix TCP buffer parsing to find JSON statements by correctly ignoring escaped quotes, and brackets {, }, [, ], when inside quote pairs.

This is a simple fix to issue #15562 - where sending some JSON requests via TCP raw sockets (that is to port 9090) either fail or hang before even being passed to rapidjson for parsing. The same JSON requests made via webserver do not have this issue.
An example request (with } in a parameter value) is

{"jsonrpc": "2.0", "params": {"directory": "/etc/abc}", "media": "files", 
"properties": ["file", "lastmodified"]}, "method": "Files.GetDirectory", "id": "libDirectory"}

This looks like standard TCP protocol parsing to me, corrcting the approach we already take, and I see no reason not to fix the issue in this way. We could do the whole thing differently of course, but that kind of refactor is best left until someone has the time and interest to do so.

@MilhouseVH care to test it out?

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

@DaveTBlake Thanks for having a go at this. I've given it a very quick test and yes, it works for the example request, when using double-quotes

However the parse errors are still there if you swap " for ' in the example request, ie.:

{'jsonrpc': '2.0', 'params': {'directory': '/storage/abc}', 'media': 'files', 'properties': ['file', 'lastmodified']}, 'method': 'Files.GetDirectory', 'id': 'libDirectory'}

as this is equally valid JSON, but fails to parse as before, so I think the inString determination will also need to consider strings enclosed by apostrophes and/or double-quotes, and requests that might use a mixture of apostrophe/double-quote strings, and of course handle apostrophes within double-quote strings (and vice versa)...

For example, this request will hang with no response (the strings all use apostrophes, but the directory property includes an embedded double-quote):

{'jsonrpc': '2.0', 'params': {'directory': '/storage/abc"', 'media': 'files', 'properties': ['file', 'lastmodified']}, 'method': 'Files.GetDirectory', 'id': 'libDirectory'}

I suppose it's also possible that a malformed request with unbalanced single/double quotes etc. might easily hang the TCP server, but I suppose that is possible already with unbalanced braces.

I'm not sure how complicated/safe/reliable this kind of single/double quote parsing might be as it's unlikely to be trivial given all the possible permutations but I'll let you be the judge of that!

@rmrector
Copy link
Contributor

left a comment

I'm cool with this quick fix in general, but I see one small gotcha.

Show resolved Hide resolved xbmc/network/TCPServer.cpp Outdated
@rmrector

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

However the parse errors are still there if you swap " for ' in the example request

@MilhouseVH Nah, that's not valid JSON, JSON keeps it simple by requiring double-quoted strings.

@pkerling

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

@DaveTBlake What do you think about adding unit tests? This is the kind of code that makes it quite easy to include some and could really benefit.

@pkerling pkerling requested a review from rmrector Feb 25, 2019

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Feb 25, 2019

Thanks for testing @MilhouseVH (you had me for a moment with ' - all my testing to pot! But no, phew)

Thanks for catch @rmrector - away from computer today, but will implement once I'm back at it.

@pkerling unit tests sounds a find idea, especially if you would like to help out with that. The test units and I have not met, let alone sending JSON via TCP using them

@pkerling

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

@pkerling unit tests sounds a find idea, especially if you would like to help out with that. The test units and I have not met, let alone sending JSON via TCP using them

I can give you some hints, sure. I think it would be fine to test just the behavior you changed here, i.e. how multiple requests are split. That is much more easily doable than going through the whole TCP stack. For that to work, I guess it would be practical to separate the request splitting part from the TCP server, so it can be tested on its own.
Let me know if you need further help.

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

@MilhouseVH Nah, that's not valid JSON, JSON keeps it simple by requiring double-quoted strings.

Thanks, I'd always thought single and double quotes were interchangeable with JSON but apparently not, so that does simplify the fix!

I'll include this change in nightlies and report any further feedback.

@rmrector rmrector removed their request for review Feb 26, 2019

Fix TCP buffer parsing to find JSON statements to correctly ignore es…
…caped quotes, and brackets when inside quote pairs.

@DaveTBlake DaveTBlake force-pushed the DaveTBlake:TCPparsefix branch from ba498e9 to 3bde7f7 Feb 26, 2019

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2019

I'll include this change in nightlies and report any further feedback.

Thanks @MilhouseVH, PR updated with Ryan's correction

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

Any objections?

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

There have been no complaints so far from any test users, so no objection from me.

@DaveTBlake DaveTBlake added this to the Leia 18.2-rc1 milestone Mar 1, 2019

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2019

Support from Millhouse and no objections, time for merge :)

@DaveTBlake DaveTBlake merged commit 1a261cf into xbmc:master Mar 4, 2019

1 check passed

default You're awesome. Have a cookie
Details

@DaveTBlake DaveTBlake deleted the DaveTBlake:TCPparsefix branch Mar 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.