Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion capa/engine.py
Original file line number Diff line number Diff line change
Expand Up @@ -197,8 +197,12 @@ def evaluate(self, features: FeatureSet, short_circuit=True):
capa.perf.counters["evaluate.feature.some"] += 1

if short_circuit:
results = []
results: list[Result] = []
satisfied_children_count = 0
if satisfied_children_count >= self.count:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

it doesn't make sense for count to be less than 0, so this condition is better expressed as if self.count == 0. if we don't have validation that count is at least 0, then we should add that in the rule loading code, not try to handle the case here.

# short circuit immediately if threshold is already met (e.g. count <= 0)
return Result(True, self, results)

for child in self.children:
result = child.evaluate(features, short_circuit=short_circuit)
results.append(result)
Expand Down
19 changes: 19 additions & 0 deletions tests/test_engine.py
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,25 @@ def test_some():
assert bool(Some(0, [Number(1)]).evaluate({Number(0): {ADDR1}})) is True
assert bool(Some(1, [Number(1)]).evaluate({Number(0): {ADDR1}})) is False

# Test Some(0, []) correctness (should evaluate to True in both short-circuit modes)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

imho this isn't a reasonable structure for a rule, so this test is noise.

assert Some(0, []).evaluate({Number(0): {ADDR1}}, short_circuit=True).success is True
assert Some(0, []).evaluate({Number(0): {ADDR1}}, short_circuit=False).success is True

# Test Some(0, [Child]) optimization (child must not be evaluated when short-circuit is True)
class TrackingFeature(Number):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

i find this to be an overly clever way to test the behavior.

i'm not sure that the behavior is worth the code maintenance investment for this test. i don't think the the behavior will likely regress. therefore i'd propose to remove these tests, too, but i'm open to discussion.

thoughts?

def __init__(self, value):
super().__init__(value)
self.evaluated = False

def evaluate(self, features, short_circuit=True):
self.evaluated = True
return super().evaluate(features, short_circuit)

tracker = TrackingFeature(1)
res = Some(0, [tracker]).evaluate({Number(0): {ADDR1}}, short_circuit=True)
assert res.success is True
assert tracker.evaluated is False # Must not be evaluated!

assert bool(Some(2, [Number(1), Number(2), Number(3)]).evaluate({Number(0): {ADDR1}})) is False
assert bool(Some(2, [Number(1), Number(2), Number(3)]).evaluate({Number(0): {ADDR1}, Number(1): {ADDR1}})) is False
assert (
Expand Down
Loading