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

Make JSON deserializer more robust #2291

Closed
lbajolet opened this issue Aug 25, 2016 · 7 comments
Closed

Make JSON deserializer more robust #2291

lbajolet opened this issue Aug 25, 2016 · 7 comments

Comments

@lbajolet
Copy link
Contributor

When working on missions, I noticed that the JSON deserializer was a bit too fragile and caused the server to crash multiple times.
When attempting to deserialize malformatted data from client, the deserializer crashes due to some asserts left in the library.

I'd propose to return nullable when deserializing, such that when a bad packet is received, the client needs to handle the possibility of a null instead of letting the whole application crash on an assert fail.
This does change the API-side a bit though.

@xymus since you are the main developer/maintainer of the library, do you have any idea to make the whole serialization/deserialization more robust while avoiding to burden the clients too much ?

@lbajolet lbajolet changed the title Make JSON deserializaer more robust Make JSON deserializer more robust Aug 25, 2016
@xymus
Copy link
Contributor

xymus commented Aug 26, 2016

What method are you thinking about? Deserializer::deserialize already returns a nullable Object.

There should not be any abort or casts left in the json::serialization module. Please send me the stacktrace if you have such an error.

As usual, don't forget to check for errors:

  • After calling Deserializer::deserialize, always check Deserializer::errors. If a single error was raised you should not trust the returned value. (This isn't true per say, some errors can be ignored, but if you want an easy deserialization, consider that any error report the data as corrupted.)
  • If you don't care for partial objects you can set Deserializer::keep_going = false, it will stop deserializing an object on the first error. But still, the even more empty objet will be returned and you must check for errors. This may make debugging easier, but usually it is more relevant for the binary deserialization.

@xymus
Copy link
Contributor

xymus commented Aug 26, 2016

By the way, deserialize returns null on 2 conditions:

  • The deserialized data is a null.
  • There was a fatal error (the JSON object was invalid or something like that).

@xymus
Copy link
Contributor

xymus commented Aug 26, 2016

Edited my previous comments after I read the doc of keep_going.

@privat
Copy link
Member

privat commented Aug 26, 2016

I had a crash recently:

import json::serialization

class A
    serialize
    var field: Int
end

var des = new JsonDeserializer("garbage")
var a = new A.from_deserializer(des)
print des.errors
print a.field
../lib/json/serialization.nit:293,3--26: Runtime error: Assert failed
            assert not path.is_empty # This is an internal error, abort
            ^
,---- Stack trace -- - -  -
| serialization$JsonDeserializer$deserialize_attribute (../lib/json/serialization.nit:293,3--26)
| x$A$from_deserializer (7,14--52)
| x$Sys$main (x.nit:9,9--36)
| kernel$Sys$run (../lib/core/kernel.nit:296,13--16)
`------------------- - -  -

@xymus
Copy link
Contributor

xymus commented Aug 26, 2016

@privat Haa, that was why we didn't use from_deserializer directly... I'll fix it, but you should still not use it when you can avoid it.

@lbajolet
Copy link
Contributor Author

What @privat reported is exactly the problem I had, admittedly the instanciation method might not be the most appropriate when dealing with unsafe sources.
I'll see what I can do to avoid using it directly in client code.

privat added a commit that referenced this issue Aug 26, 2016
This error could arise when parsing invalid JSON strings (or non-object JSON values) and skipping the call to `deserialize`.

See @privat comment in #2291.

Pull-Request: #2295
Reviewed-by: Jean Privat <[email protected]>
Reviewed-by: Lucas Bajolet <[email protected]>
@xymus
Copy link
Contributor

xymus commented Aug 27, 2016

Was it a single bug? is it fixed by #2295?

@xymus xymus closed this as completed Sep 6, 2016
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

3 participants