Skip to content

fix: provide ResultSize for string.format() cost estimate#1322

Open
Dhakshin2007 wants to merge 1 commit into
cel-expr:masterfrom
Dhakshin2007:master
Open

fix: provide ResultSize for string.format() cost estimate#1322
Dhakshin2007 wants to merge 1 commit into
cel-expr:masterfrom
Dhakshin2007:master

Conversation

@Dhakshin2007

Copy link
Copy Markdown

Prior to this fix, the ExtFormatString cost case returned no ResultSize, with the comment "ResultSize not calculated because we can't bound the max size."

Since PR #1292 introduced StringsMaxPrecision (defaulting to 100 for v5+), the output size of string.format() is now bounded: each format clause expands an argument by at most maxPrecision + small constant characters. The output is therefore bounded by the format string size plus the sum of argument string sizes.

This change computes ResultSize = targetSize + argsSize, enabling downstream cost-tracking expressions (e.g. format output passed to contains(), size(), or string concatenation) to have accurate cost estimates, which is important for enforcing compute limits in security-sensitive CEL evaluation contexts.

Pull Requests Guidelines

See CONTRIBUTING.md for more details about when to create
a GitHub Pull Request and when other kinds of contributions or
consultation might be more desirable.

When creating a new pull request, please fork the repo and work within a
development branch.

Commit Messages

  • Most changes should be accompanied by tests.
  • Commit messages should explain why the changes were made.
Summary of change in 50 characters or less

Background on why the change is being made with additional detail on
consequences of the changes elsewhere in the code or to the general
functionality of the library. Multiple paragraphs may be used, but
please keep lines to 72 characters or less.

Reviews

  • Perform a self-review.
  • Make sure the Travis CI build passes.
  • Assign a reviewer once both the above have been completed.

Merging

  • If a CEL maintaner approves the change, it may be merged by the author if
    they have write access. Otherwise, the change will be merged by a maintainer.
  • Multiple commits should be squashed before merging.
  • Please append the line closes #<issue-num>: description in the merge message,
    if applicable.

Prior to this fix, the ExtFormatString cost case returned no ResultSize,
with the comment "ResultSize not calculated because we can't bound the
max size."

Since PR cel-expr#1292 introduced StringsMaxPrecision (defaulting to 100 for
v5+), the output size of string.format() is now bounded: each format
clause expands an argument by at most maxPrecision + small constant
characters. The output is therefore bounded by the format string size
plus the sum of argument string sizes.

This change computes ResultSize = targetSize + argsSize, enabling
downstream cost-tracking expressions (e.g. format output passed to
contains(), size(), or string concatenation) to have accurate cost
estimates, which is important for enforcing compute limits in
security-sensitive CEL evaluation contexts.
@TristonianJones

Copy link
Copy Markdown
Collaborator

/gcbrun

Comment thread checker/cost.go
case overloads.ExtFormatString:
if target != nil {
// ResultSize not calculated because we can't bound the max size.
// ResultSize is bounded: since precision is capped by StringsMaxPrecision, // the output cannot exceed the format string size plus the sum of // argument string sizes (each numeric arg expands by at most // maxPrecision + fixed overhead characters).

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.

Unfortunately, I can't change these lines and instead would need to move the existing cost caculations into the ext/strings.go implementation and associate it with v0-v4 of the library, and the update written here with the v5+ versions of the library

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