Improve Serialization Logic #82
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
feanil
force-pushed
the
feanil/zip_debug
branch
from
0b5f901 to
f58127e
Compare
As a part of the python3 conversion, we added logic to try to convert bytestrings to unicode strings. This had a problem because not all bytestrings that are produced as a part of the context passed into and out of the jailed code execution is actually unicode strings. In some cases this was resulting in a unicode decode error. Before this code existed we used to just drop any nonstandard types and also only keep complex types(list,dict,tuples) that were easily jsonable. In python2 this was fine because byte strings would automatically get coerced to unicode strings. However in python3 this will throw errors. So if we don't explicitly try to convert all the byte strings to unicode, we could stop passing back data that we previously used to pass in. The first iteration of this code assumed all byte arrays were unicode strings. From evidence in production this is not always the case. So we fall back to the behaviour we had before this change where if an item can't successfully be converted to something we can serialize to json, we just drop that item from the list of content being passed back through the serialization straw.
feanil
force-pushed
the
feanil/zip_debug
branch
from
f58127e to
d84f922
Compare
feanil
changed the title
jmbowman
reviewed
Sep 26, 2019
codejail/safe_exec.py
Outdated
| @@ -22,7 +22,7 @@ | |||
| # Flags to let developers temporarily change some behavior in this file. | |||
|
|
|||
| # Set this to True to log all the code and globals being executed. | |||
| LOG_ALL_CODE = False | |||
| LOG_ALL_CODE = True | |||
There was a problem hiding this comment.
I presume this change should be reverted before merging?
codejail/safe_exec.py
Outdated
| try: | ||
| new_value = filter_unserializable(v) | ||
| if jsonable(new_value): | ||
| new_dict[k] = new_value |
codejail/safe_exec.py
Outdated
|
|
||
| ok_types = (type(None), int, float, str, six.text_type, list, tuple, dict) | ||
| serializable_dict = filter_unserializable(d) | ||
| #serializable_dict = d |
codejail/tests/test_safe_exec.py
Outdated
| globs | ||
| ) | ||
| from pprint import pprint | ||
| pprint(globs) |
jmbowman
approved these changes
Sep 26, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As a part of the python3 conversion, we added logic to try to convert
bytestrings to unicode strings. This had a problem because not all
bytestrings that are produced as a part of the context passed into and
out of the jailed code execution is actually unicode strings. In some
cases this was resulting in a unicode decode error.
Before this code existed we used to just drop any nonstandard types and
also only keep complex types(list,dict,tuples) that were easily
jsonable. In python2 this was fine because byte strings would
automatically get coerced to unicode strings. However in python3 this
will throw errors. So if we don't explicitly try to convert all the
byte strings to unicode, we could stop passing back data that we
previously used to pass in.
The first iteration of this code assumed all byte arrays were unicode
strings. From evidence in production this is not always the case. So
we fall back to the behaviour we had before this change where if an item
can't successfully be converted to something we can serialize to json,
we just drop that item from the list of content being passed back
through the serialization straw.