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.

Sign up for GitHub

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

Jump to bottom

Python 3 - pathod.app #1170

Closed
wants to merge 6 commits into from
Closed

Python 3 - pathod.app #1170

wants to merge 6 commits into from

Conversation

dufferzafar
Copy link
Contributor

@dufferzafar dufferzafar commented May 27, 2016 •
edited

Fixing errors that came up when trying to make the test/pathod/test_app.py tests pass with Py3.

dufferzafar
dufferzafar reviewed May 28, 2016
View reviewed changes
pathod/language/__init__.py
@@ -26,7 +26,7 @@ def parse_pathod(s, use_http2=False):
May raise ParseException
"""
try:
s = s.decode("ascii")
s.encode("ascii")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're only checking whether the string is valid ASCII here, so encode would be better since it works on both Python versions. Right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@dufferzafar
Copy link
Contributor Author

dufferzafar commented May 28, 2016 •
edited

After making these changes, the tests in files test_patho{c,d}_cmdline.py pass too. Should I enable Travis for them in this pull itself?

@mhils
Copy link
Member

mhils commented May 28, 2016

Go for it! 😃

@dufferzafar
Copy link
Contributor Author

I'm stuck on the currently failing tests so can you please review and provide some hints as to how I can fix them?

@Kriechi
Copy link
Member

Kriechi commented May 29, 2016

@dufferzafar please rebase to current master - there are a few changes there which might help debug this. Thanks.

@Kriechi
Copy link
Member

Kriechi commented May 29, 2016

Looks like you need to properly encode PathodHandler.settings.
These settings get passed as str to crafted.freeze(self.settings)
and from there on it bubbles through multiple function, to finally explode in pathod/language/base.py:169 at return TokValueLiteral(bytes_to_escaped_str(g[:])).

@dufferzafar dufferzafar force-pushed the pathod-app branch 2 times, most recently from 312e423 to 9b8aeac Compare May 31, 2016 16:22
@dufferzafar
Copy link
Contributor Author

dufferzafar commented May 31, 2016 •
edited

Correct me if I'm wrong, but I think the problem is in the freeze functions not returning correct type of data. This datatypes dict needs to be converted to bytes, I guess

The same types of errors also seem to be occurring in pathod.language.base.

@Kriechi
Copy link
Member

Kriechi commented May 31, 2016

Yes - looks reasonable. Can you try and make the changes? If this is to complex, we might need to explore other approaches.

@mhils
Copy link
Member

mhils commented May 31, 2016 •
edited

@dufferzafar: Appending .encode() works for all the string.* values, for bytes we can use

mitmproxy/test/pathod/test_utils.py

Lines 26 to 28 in a7abf8b

s = bytes(range(256))
if six.PY2:
s = "".join([chr(i) for i in range(255)])
again.

dufferzafar
dufferzafar reviewed Jun 1, 2016
View reviewed changes
pathod/language/generators.py
@@ -55,12 +55,12 @@ def __len__(self):
return self.length

def __getitem__(self, x):
return random.choice(DATATYPES[self.dtype])
return random.choice(DATATYPES[self.dtype]).encode()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mhils Instead of encoding values in the DATAYPES dict, we could do this, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could, but I think we should tackle the problem at the source and fix DATATYPES directly.

@dufferzafar
Copy link
Contributor Author

dufferzafar commented Jun 1, 2016 •
edited

Still can't make sense of the failing tests 😭

@Kriechi
Copy link
Member

Kriechi commented Jun 1, 2016

So I guess the test failures happen due to the tracebacks where one of the threads just silently fails (without raising a real error, but just printing the traceback and exiting cleanly).

I think this is the core problem:

  File "/home/travis/build/mitmproxy/mitmproxy/pathod/pathod.py", line 110, in http_serve_crafted
    self.settings
  File "/home/travis/build/mitmproxy/mitmproxy/pathod/language/__init__.py", line 119, in serve
    ret.update(msg.log(settings))
  File "/home/travis/build/mitmproxy/mitmproxy/pathod/language/message.py", line 82, in log
    v = [x[:LOG_TRUNCATE] for x in v.values(settings)]
  File "/home/travis/build/mitmproxy/mitmproxy/pathod/language/generators.py", line 63, in __getslice__
    return "".join(random.choice(chars).encode() for x in range(a, b))
  File "/home/travis/build/mitmproxy/mitmproxy/pathod/language/generators.py", line 63, in <genexpr>
    return "".join(random.choice(chars).encode() for x in range(a, b))
UnicodeDecodeError: 'ascii' codec can't decode byte 0xf3 in position 0: ordinal not in range(128)

mhils
mhils reviewed Jun 1, 2016
View reviewed changes
pathod/pathod.py
@@ -4,7 +4,8 @@
import os
import sys
import threading
import urllib

from six.moves.urllib.parse import unquote
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cortesi and I discussed yesterday that we do not want to import individual functions (as per https://google.github.io/styleguide/pyguide.html). Can we change this to from six.moves import urllib?

@dufferzafar dufferzafar mentioned this pull request Jun 2, 2016
Merged
@dufferzafar
Copy link
Contributor Author

dufferzafar commented Jun 2, 2016 •
edited

Will have to be rebased (to remove some conflicts) once #1195 gets merged.

@Kriechi
Copy link
Member

Kriechi commented Jun 2, 2016

@dufferzafar #1195 is merged! Great work so far - I hope we don't loose track of all those PRs you are sending 👍

@dufferzafar
Copy link
Contributor Author

Stalled because it depends on a lot of pathod stuff that hasn't been ported yet.

@dufferzafar dufferzafar force-pushed the pathod-app branch 2 times, most recently from 38abd20 to b07d32f Compare June 4, 2016 10:58
@dufferzafar
Copy link
Contributor Author

Closing this, since #1211 will remove the pathod.app module.

@dufferzafar dufferzafar closed this Jun 5, 2016
@dufferzafar dufferzafar deleted the pathod-app branch June 5, 2016 18:02
@dufferzafar dufferzafar modified the milestone: python-3 Jul 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
python-3
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dufferzafar @mhils @Kriechi