Skip to content

fix(builtins): prevent panic in strings.count on empty needle - #796

Open
Anand Krishnamoorthi (anakrish) wants to merge 1 commit into
microsoft:mainfrom
anakrish:fix/strings-count-empty-needle-panic
Open

fix(builtins): prevent panic in strings.count on empty needle#796
Anand Krishnamoorthi (anakrish) wants to merge 1 commit into
microsoft:mainfrom
anakrish:fix/strings-count-empty-needle-panic

Conversation

@anakrish

Copy link
Copy Markdown
Collaborator

Summary

strings.count panicked at runtime when called with an empty needle string because .windows(0) is not valid in Rust. This PR fixes three tightly coupled issues in strings_count:

Bugs fixed

# Bug Impact
1 Wrong param index (params[0] used for substring error span instead of params[1]) Misleading error messages
2 Panic: .windows(needle.len()) panics when needle is empty (len == 0) Process crash
3 Semantics: byte-level .windows() counted overlapping matches; OPA uses non-overlapping semantics (Go strings.Count) Wrong results, e.g. strings.count("aaaa", "aa") returned 3 instead of 2

Fix

// Before (bugs):
let substring = ensure_string(name, &params[0], &args[1])?;  // wrong index
Ok(Value::from(
    search.as_bytes().windows(substring.len())  // panics on empty
        .filter(|&w| w == substring.as_bytes()).count()  // overlapping
))

// After:
let substring = ensure_string(name, &params[1], &args[1])?;
if substring.is_empty() {
    // Empty needle: matches between every character and at both ends
    // consistent with Go strings.Count and OPA semantics
    return Ok(Value::from(search.chars().count().saturating_add(1)));
}
Ok(Value::from(search.matches(substring.as_ref()).count()))

Tests

A YAML regression test is added at tests/interpreter/cases/builtins/strings/strings_count.yaml covering:

  • Basic match, no-match, multi-match
  • Empty needle on non-empty string (was panic, now returns char_count + 1)
  • Empty needle on empty string (returns 1)
  • Non-overlapping semantics (strings.count("aaaa", "aa") = 2, not 3)
  • Multi-byte UTF-8 string with empty needle

strings.count panicked when called with an empty needle because
.windows(0) is invalid. Fix three issues in strings_count:

1. Wrong param index: params[0] was used for the substring error span
   instead of params[1].
2. Panic: .windows(needle.len()) panics when needle is empty (len == 0).
3. Semantics: the byte-level .windows() approach counted overlapping
   matches; OPA uses non-overlapping match semantics (Go strings.Count).

The fix:
- Correct the param index to params[1] for the substring argument.
- Handle empty needle explicitly: return chars().count() + 1, matching
  Go's strings.Count behaviour (empty needle matches between every
  character and at both ends).
- Use str::matches() for non-overlapping count on non-empty needles.

A YAML regression test is added covering basic match, no-match,
multi-match, empty-needle (must not panic), empty-needle on empty
string, non-overlapping semantics, and multi-byte UTF-8 strings.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@anakrish
Anand Krishnamoorthi (anakrish) force-pushed the fix/strings-count-empty-needle-panic branch from 67aadd3 to e0e0e31 Compare August 28, 2026 15:34
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.

1 participant