Skip to content

Conversation

KevinHock
Copy link
Collaborator

@KevinHock KevinHock commented Mar 18, 2017

This PR started as adding support for relative imports, but naturally became an inter-procedural PR including tests for things I haven't done yet like init files, __all__, from foo import * with style and random improvements e.g. project_handler_test along the way.

A few of the small things:

  • get_python_modules -> get_modules because I added get_modules_and_packages

  • is_python_module -> is_python_file

  • add all tuples to project handler test

  • double-quotes all around, for consistent multi-line comments

pyt/base_cfg.py Outdated
@@ -252,6 +252,7 @@ def connect_control_flow_node(self, control_flow_node, next_node):
def connect_nodes(self, nodes):
"""Connect the nodes in a list linearly."""
for n, next_node in zip(nodes, nodes[1:]):

Copy link
Collaborator Author

@KevinHock KevinHock Apr 8, 2017

Choose a reason for hiding this comment

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

extra line, I'll remove it when I make other feedback changes

@@ -286,7 +287,7 @@ def stmt_star_handler(self, stmts):
elif isinstance(node, BreakNode):
break_nodes.append(node)

if self.node_to_connect(node):
if self.node_to_connect(node) and node:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

connect_nodes messes up if node is None

n = RestoreNode(temp_name + ' = ' + label_visitor.result,
temp_name,
[label_visitor.result],
rhs_visitor.result,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was the bug that messed up inter-procedural analysis.

for k in local_definitions.import_alias_mapping.keys():
if _id.startswith(k):
real_id = _id.replace(k, local_definitions.import_alias_mapping[k])
definition = local_definitions.get_definition(real_id)
Copy link
Collaborator Author

@KevinHock KevinHock Apr 8, 2017

Choose a reason for hiding this comment

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

This makes sure, where import bar as foo, if it sees foo it gets the definition of bar.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice one :)


# This fails due to a false positive in get_vulnerability
# def test_absolute_from_file_does_not_exist(self):
# vulnerability_log = self.run_analysis('example/vulnerable_code_across_files/absolute_from_file_does_not_exist.py')
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So when a function has to be treated as a blackbox it shouldn't assume the return value is tainted, but it's somewhat low priority to fix, for me, personally.

# class NestedTest(BaseTestCase):
# def test_nested_function_calls(self):

# path = os.path.normpath('example/nested_functions_code/nested_function_calls.py')
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not gonna fix it in this PR, I wanted to share the test though.

for node, expected_label in zip(self.cfg.nodes, EXPECTED):
self.assertEqual(node.label, expected_label)

# def test_init(self):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As I started to try to get this to pass I thought the PR would be too big so I just pushed and wrote

raise Exception("TODO: Handle __init__ files")

@KevinHock KevinHock changed the title Support for beautiful relative imports (Ready for review) Support for beautiful relative imports Apr 8, 2017
Copy link
Contributor

@Thalmann Thalmann left a comment

Choose a reason for hiding this comment

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

A lot of good stuff - good work.

@Thalmann Thalmann merged commit 09a18f9 into python-security:master Apr 16, 2017
@KevinHock
Copy link
Collaborator Author

Thanks a bunch 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants