Skip to content

Fix inequality port lookups #834

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

Merged
merged 6 commits into from
Apr 22, 2020
Merged

Fix inequality port lookups #834

merged 6 commits into from
Apr 22, 2020

Conversation

mavam
Copy link
Member

@mavam mavam commented Apr 22, 2020

This PR fixes a bug when performing queries of the form x != 80/tcp. The query was falsely evaluated as x != 80/? && x != ?/tcp. (The syntax in the second predicate does not yet exist.) The correct evaluation should be x != 80/? || x != ?/tcp, i.e., we want to include 80/udp and 80/? in the result as well, which requires performing a disjunction of the port type.

@mavam mavam marked this pull request as ready for review April 22, 2020 09:17
@mavam mavam requested a review from a team April 22, 2020 09:18
@mavam mavam added the bug Incorrect behavior label Apr 22, 2020
Copy link
Member

@lava lava left a comment

Choose a reason for hiding this comment

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

Ideally we should also add a unit (integration?) test that actually tests the situation described in the changelog, i.e. a database containing two entries "80/udp" and "8080/tcp" and a check that vast count "port != 80/tcp" == 2, the one you added could silently decay when an index other than port_index is used in the future.

@mavam
Copy link
Member Author

mavam commented Apr 22, 2020

I've added to more integration tests. The testing you are describing is actually already covered by the unit tests, I think.

@mavam mavam merged commit 9882c2f into master Apr 22, 2020
@mavam mavam deleted the story/ch14848 branch April 22, 2020 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants