> On Oct. 9, 2015, 11:14 p.m., Adam B wrote:
> > src/common/resources.cpp, lines 368-384
> > <https://reviews.apache.org/r/39018/diff/4/?file=1092358#file1092358line368>
> >
> >     This smells like a hack to workaround a bug in picojson. Can you link 
> > to something that indicates this as best practice or a recommended 
> > workaround? Is there a picojson issue we can track to know if this ever 
> > gets fixed?
> 
> Greg Mann wrote:
>     I'll have to do some research on this one; there are no existing picojson 
> issues addressing this (only 27 issues have ever been raised in the project 
> :-), but I can certainly file one. I'll get back to you on this.
> 
> Joseph Wu wrote:
>     TLDR: Our bug, not a PicoJson bug.  (Good catch though!)
>     
>     We should have a check after parsing: 
> https://github.com/apache/mesos/blob/master/3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp#L740
>     
>     `picojson::parse` will return the current position of stream iterator.  
> We should be saving this value and checking to see if it matches the end 
> (minus whitespace).
>     See: 
> https://github.com/kazuho/picojson/blob/25fc213cca61ea22b3c2e4db8def9927562ba5f7/picojson.h#L925
>     
>     Note that the reason PicoJson does not barf on that input is because 
> PicoJson supports "streaming" JSON objects.  i.e. You can give it a "stream" 
> like `{} {} {}`, and expect three objects if you parse three times (passing 
> the iterator along).

Awesome, thanks for solving that Joseph! After looking through the picojson 
source, it seems like we could be using the two-argument `picojson::parse` that 
operates on a `std::string` 
(https://github.com/kazuho/picojson/blob/master/picojson.h#L938-L942), rather 
than the four-argument function we're calling.

In the `picojson::parse()` signatures that take iterators as input, it makes 
sense to return successfully if a valid JSON value is parsed, regardless of 
what comes afterward. However, in the two-argument `std::string` case it only 
makes sense for the input string to contain a single JSON value, and it seems 
to me that proper validation would return an error if non-whitespace trailing 
characters are found.

I filed a pull request for picojson 
(https://github.com/kazuho/picojson/pull/70), and if it's merged we could 
switch to the two-argument function call and forego any additional validation 
of our own. In the meantime, I opened a JIRA at 
https://issues.apache.org/jira/browse/MESOS-3698 to track the validation 
change, and I posted a review that adds this error checking into JSON::parse(), 
since one of the tests in this patch will not pass without it.


- Greg


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review102110
-----------------------------------------------------------


On Oct. 11, 2015, 2:09 a.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
> 
> (Updated Oct. 11, 2015, 2:09 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael 
> Park.
> 
> 
> Bugs: MESOS-2467
>     https://issues.apache.org/jira/browse/MESOS-2467
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This includes code changes necessary for JSON parsing of Resources. 
> Documentation changes will be posted soon in another review 
> (https://reviews.apache.org/r/39102/).
> 
> 
> Diffs
> -----
> 
>   include/mesos/resources.hpp 6c3a065945eb56dc88df9c977e5ca11d4cbcbf61 
>   include/mesos/v1/resources.hpp fe8925ac851b74d1b37919f00afc7ed816f47ea5 
>   src/common/resources.cpp 601388c35a1bff37c58e753d1870d53b8d0af2d1 
>   src/tests/resources_tests.cpp 6584fc6c39e6ffe9f8085576677dcc669f127697 
>   src/v1/resources.cpp dc868903472f8f3a1ddc56092e3f8f81d953ce39 
> 
> Diff: https://reviews.apache.org/r/39018/diff/
> 
> 
> Testing
> -------
> 
> New code was added to `ResourcesTest.ParsingFromJSON`, and two new tests were 
> added: `ResourcesTest.ParsingFromJSONWithRoles` and 
> `ResourcesTest.ParsingFromJSONError`. These attempt to test common error 
> scenarios that might show up in user-supplied JSON.
> 
> `make check` was used to confirm that all tests pass, with one notable 
> failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue 
> tracked here: https://issues.apache.org/jira/browse/MESOS-3698
> 
> The original resources parsing format is used throughout many other tests 
> (check `src/tests/sorter_tests.cpp`, for example), so it's clear that the 
> original parsing continues to work correctly.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Reply via email to