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

Capitalisation typo in expr visitor? #146

Open
bcaller opened this issue Jul 16, 2018 · 2 comments
Open

Capitalisation typo in expr visitor? #146

bcaller opened this issue Jul 16, 2018 · 2 comments

Comments

@bcaller
Copy link
Collaborator

bcaller commented Jul 16, 2018

pyt/pyt/cfg/expr_visitor.py

Lines 207 to 209 in e692581

if (type(node) == AssignmentNode or
type(node) == AssignmentCallNode or
type(Node) == BBorBInode)]: # type() is used on purpose here

I'm not that sure what the correct behaviour here is.

Capitalisation looks wrong so I think type(Node) == BBorBInode will always be False. Removing the check on L209 doesn't affect the tests.

However, changing it to type(node) causes 15 unit test failures.

FAIL: test_call_with_attribute (tests.cfg.cfg_test.CFGCallWithAttributeTest)
AssertionError: 14 != 16
FAIL: test_function_line_numbers (tests.cfg.cfg_test.CFGFunctionNodeTest)
AssertionError: 1 != None
FAIL: test_function_parameters (tests.cfg.cfg_test.CFGFunctionNodeTest)
AssertionError: 14 != 16
FAIL: test_function_with_return (tests.cfg.cfg_test.CFGFunctionNodeTest)
AssertionError: 19 != 25
FAIL: test_multiple_blackbox_calls_in_user_defined_call_after_if (tests.cfg.cfg_test.CFGFunctionNodeTest)
AssertionError: 32 != 38
FAIL: test_multiple_nested_user_defined_calls_after_if (tests.cfg.cfg_test.CFGFunctionNodeTest)
AssertionError: 39 != 45
FAIL: test_multiple_user_defined_calls_in_blackbox_call_after_if (tests.cfg.cfg_test.CFGFunctionNodeTest)
AssertionError: 30 != 36
FAIL: test_simple_function (tests.cfg.cfg_test.CFGFunctionNodeTest)
AssertionError: 9 != 11
FAIL: test_orelse (tests.cfg.cfg_test.CFGTryTest)
AssertionError: 20 != 26
FAIL: test_orelse_with_no_variables_to_save (tests.cfg.cfg_test.CFGTryTest)
AssertionError: 15 != 19
FAIL: test_try_orelse_with_no_variables_to_save_and_no_args (tests.cfg.cfg_test.CFGTryTest)
AssertionError: 13 != 17
FAIL: test_from_package_import_star (tests.cfg.import_test.ImportTest)
AssertionError: 'save_3_~call_2 = ~call_2' != 'Function Entry B.al'
FAIL: test_from_package_import_star_with_alias (tests.cfg.import_test.ImportTest)
AssertionError: 'save_3_~call_2 = ~call_2' != 'Function Entry meringue.al'
FAIL: test_ensure_saved_scope (tests.vulnerabilities.vulnerabilities_test.EngineTest)
AssertionError: False is not true
FAIL: test_path_traversal_result (tests.vulnerabilities.vulnerabilities_test.EngineTest)
AssertionError: False is not true
FAILED (failures=15)
@KevinHock
Copy link
Collaborator

Great catch! I probably should have used in tuple. I'll have to have a deeper look into to see if I should remove it or fix the typo/tests but I'll fix the typo/tests.

@adrianbn
Copy link
Contributor

adrianbn commented Dec 8, 2018

Reading a bit on this code it does look like the correct fix here is to change the capitalization. As it stands now we're not saving the scope for any assignment that returns from a blackbox call.

I reckon this will be a bit tedious to fix since the change makes a bunch of the cfg tests fail because now they have more nodes in the CFG.

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

No branches or pull requests

3 participants