Skip to content

Commit 8e6184f

Browse files
jnthntatumcopybara-github
jnthntatum
authored and
copybara-github
committed
Add option for format precision limit.
High values for precision can lead to very large strings (exponential time / memory cost w.r.t format specifier), but are technically allowed. Will lower the default value in a follow-up. PiperOrigin-RevId: 888348453
1 parent 1792682 commit 8e6184f

7 files changed

Lines changed: 146 additions & 39 deletions

File tree

‎env/runtime_std_extensions.cc‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,8 +105,11 @@ void RegisterStandardExtensions(EnvRuntime& env_runtime) {
105105
"cel.lib.ext.strings", "strings", version,
106106
[version](RuntimeBuilder& runtime_builder,
107107
const RuntimeOptions& runtime_options) -> absl::Status {
108+
cel::extensions::StringsExtensionOptions strings_options;
109+
strings_options.version = version;
108110
return cel::extensions::RegisterStringsFunctions(
109-
runtime_builder.function_registry(), runtime_options, version);
111+
runtime_builder.function_registry(), runtime_options,
112+
strings_options);
110113
});
111114
}
112115

‎extensions/formatting.cc‎

Lines changed: 32 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -14,20 +14,19 @@
1414

1515
#include "extensions/formatting.h"
1616

17+
#include <algorithm>
1718
#include <cmath>
1819
#include <cstdint>
1920
#include <limits>
2021
#include <memory>
2122
#include <optional>
2223
#include <string>
23-
#include <tuple>
2424
#include <type_traits>
2525
#include <utility>
2626

2727
#include "absl/base/attributes.h"
2828
#include "absl/base/nullability.h"
2929
#include "absl/container/btree_map.h"
30-
#include "absl/memory/memory.h"
3130
#include "absl/numeric/bits.h"
3231
#include "absl/status/status.h"
3332
#include "absl/status/statusor.h"
@@ -64,7 +63,7 @@ absl::StatusOr<absl::string_view> FormatString(
6463
std::string& scratch ABSL_ATTRIBUTE_LIFETIME_BOUND);
6564

6665
absl::StatusOr<std::pair<int64_t, std::optional<int>>> ParsePrecision(
67-
absl::string_view format) {
66+
absl::string_view format, int max_precision) {
6867
if (format.empty() || format[0] != '.') return std::pair{0, std::nullopt};
6968

7069
int64_t i = 1;
@@ -80,9 +79,9 @@ absl::StatusOr<std::pair<int64_t, std::optional<int>>> ParsePrecision(
8079
return absl::InvalidArgumentError(
8180
"unable to convert precision specifier to integer");
8281
}
83-
if (precision > kMaxPrecision) {
82+
if (precision > max_precision) {
8483
return absl::InvalidArgumentError(
85-
absl::StrCat("precision specifier exceeds maximum of ", kMaxPrecision));
84+
absl::StrCat("precision specifier exceeds maximum of ", max_precision));
8685
}
8786
return std::pair{i, precision};
8887
}
@@ -444,12 +443,13 @@ absl::StatusOr<absl::string_view> FormatScientific(
444443
}
445444

446445
absl::StatusOr<std::pair<int64_t, absl::string_view>> ParseAndFormatClause(
447-
absl::string_view format, const Value& value,
446+
absl::string_view format, const Value& value, int max_precision,
448447
const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool,
449448
google::protobuf::MessageFactory* absl_nonnull message_factory,
450449
google::protobuf::Arena* absl_nonnull arena,
451450
std::string& scratch ABSL_ATTRIBUTE_LIFETIME_BOUND) {
452-
CEL_ASSIGN_OR_RETURN(auto precision_pair, ParsePrecision(format));
451+
CEL_ASSIGN_OR_RETURN(auto precision_pair,
452+
ParsePrecision(format, max_precision));
453453
auto [read, precision] = precision_pair;
454454
switch (format[read]) {
455455
case 's': {
@@ -494,7 +494,7 @@ absl::StatusOr<std::pair<int64_t, absl::string_view>> ParseAndFormatClause(
494494
}
495495

496496
absl::StatusOr<Value> Format(
497-
const StringValue& format_value, const ListValue& args,
497+
const StringValue& format_value, const ListValue& args, int max_precision,
498498
const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool,
499499
google::protobuf::MessageFactory* absl_nonnull message_factory,
500500
google::protobuf::Arena* absl_nonnull arena) {
@@ -512,43 +512,50 @@ absl::StatusOr<Value> Format(
512512
}
513513
++i;
514514
if (i >= format.size()) {
515-
return absl::InvalidArgumentError("unexpected end of format string");
515+
return ErrorValue(
516+
absl::InvalidArgumentError("unexpected end of format string"));
516517
}
517518
if (format[i] == '%') {
518519
result.push_back('%');
519520
continue;
520521
}
521522
if (arg_index >= args_size) {
522-
return absl::InvalidArgumentError(
523-
absl::StrFormat("index %d out of range", arg_index));
523+
return ErrorValue(absl::InvalidArgumentError(
524+
absl::StrFormat("index %d out of range", arg_index)));
524525
}
525526
CEL_ASSIGN_OR_RETURN(auto value, args.Get(arg_index++, descriptor_pool,
526527
message_factory, arena));
527-
CEL_ASSIGN_OR_RETURN(
528-
auto clause,
529-
ParseAndFormatClause(format.substr(i), value, descriptor_pool,
530-
message_factory, arena, clause_scratch));
531-
absl::StrAppend(&result, clause.second);
532-
i += clause.first;
528+
529+
auto clause = ParseAndFormatClause(format.substr(i), value, max_precision,
530+
descriptor_pool, message_factory, arena,
531+
clause_scratch);
532+
if (!clause.ok()) {
533+
return ErrorValue(std::move(clause).status());
534+
}
535+
absl::StrAppend(&result, clause->second);
536+
i += clause->first;
533537
}
534-
return StringValue(arena, std::move(result));
538+
return StringValue::From(std::move(result), arena);
535539
}
536540

537541
} // namespace
538542

539543
absl::Status RegisterStringFormattingFunctions(FunctionRegistry& registry,
540-
const RuntimeOptions& options) {
544+
const RuntimeOptions& options,
545+
int max_precision) {
546+
max_precision = std::clamp(max_precision, 0, kMaxPrecision);
541547
CEL_RETURN_IF_ERROR(registry.Register(
542548
BinaryFunctionAdapter<absl::StatusOr<Value>, StringValue, ListValue>::
543549
CreateDescriptor("format", /*receiver_style=*/true),
544550
BinaryFunctionAdapter<absl::StatusOr<Value>, StringValue, ListValue>::
545551
WrapFunction(
546-
[](const StringValue& format, const ListValue& args,
547-
const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool,
548-
google::protobuf::MessageFactory* absl_nonnull message_factory,
549-
google::protobuf::Arena* absl_nonnull arena) {
550-
return Format(format, args, descriptor_pool, message_factory,
551-
arena);
552+
[max_precision](
553+
const StringValue& format, const ListValue& args,
554+
const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool,
555+
google::protobuf::MessageFactory* absl_nonnull message_factory,
556+
google::protobuf::Arena* absl_nonnull arena) {
557+
return Format(format, args, max_precision, descriptor_pool,
558+
message_factory, arena);
552559
})));
553560
return absl::OkStatus();
554561
}

‎extensions/formatting.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,8 @@ namespace cel::extensions {
2323

2424
// Register extension functions for string formatting.
2525
absl::Status RegisterStringFormattingFunctions(FunctionRegistry& registry,
26-
const RuntimeOptions& options);
26+
const RuntimeOptions& options,
27+
int max_precision = 1000);
2728

2829
} // namespace cel::extensions
2930

‎extensions/formatting_test.cc‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,30 @@ TEST_P(StringFormatLimitsTest, FormatLimits) {
9696
}
9797
}
9898

99+
TEST(StringFormatLimitsTest, MaxPrecisionOption) {
100+
google::protobuf::Arena arena;
101+
const RuntimeOptions options;
102+
ASSERT_OK_AND_ASSIGN(auto builder,
103+
CreateStandardRuntimeBuilder(
104+
internal::GetTestingDescriptorPool(), options));
105+
ASSERT_THAT(RegisterStringFormattingFunctions(builder.function_registry(),
106+
options, /*max_precision=*/99),
107+
IsOk());
108+
109+
ASSERT_OK_AND_ASSIGN(auto runtime, std::move(builder).Build());
110+
111+
ASSERT_OK_AND_ASSIGN(ParsedExpr expr, Parse("'%.100f'.format([1.123])",
112+
"<input>", ParserOptions{}));
113+
ASSERT_OK_AND_ASSIGN(std::unique_ptr<Program> program,
114+
ProtobufRuntimeAdapter::CreateProgram(*runtime, expr));
115+
Activation activation;
116+
117+
ASSERT_OK_AND_ASSIGN(Value value, program->Evaluate(&arena, activation));
118+
ASSERT_TRUE(value.Is<ErrorValue>());
119+
EXPECT_THAT(value.GetError().ToStatus().message(),
120+
HasSubstr("precision specifier exceeds maximum of 99"));
121+
}
122+
99123
INSTANTIATE_TEST_SUITE_P(StringFormatLimitsTest, StringFormatLimitsTest,
100124
ValuesIn<std::string>({
101125
"double('%.326f'.format([x])) == x",

‎extensions/strings.cc‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -305,9 +305,10 @@ absl::Status RegisterStringsDecls(TypeCheckerBuilder& builder, int version) {
305305

306306
} // namespace
307307

308-
absl::Status RegisterStringsFunctions(FunctionRegistry& registry,
309-
const RuntimeOptions& options,
310-
int version) {
308+
absl::Status RegisterStringsFunctions(
309+
FunctionRegistry& registry, const RuntimeOptions& options,
310+
const StringsExtensionOptions& extension_options) {
311+
const int version = extension_options.version;
311312
CEL_RETURN_IF_ERROR(registry.Register(
312313
BinaryFunctionAdapter<absl::StatusOr<Value>, StringValue, StringValue>::
313314
CreateDescriptor("split", /*receiver_style=*/true),
@@ -382,7 +383,8 @@ absl::Status RegisterStringsFunctions(FunctionRegistry& registry,
382383
return absl::OkStatus();
383384
}
384385

385-
CEL_RETURN_IF_ERROR(RegisterStringFormattingFunctions(registry, options));
386+
CEL_RETURN_IF_ERROR(RegisterStringFormattingFunctions(
387+
registry, options, extension_options.max_precision));
386388
CEL_RETURN_IF_ERROR(
387389
(UnaryFunctionAdapter<StringValue, StringValue>::RegisterGlobalOverload(
388390
"strings.quote", &Quote, registry)));
@@ -412,13 +414,16 @@ absl::Status RegisterStringsFunctions(FunctionRegistry& registry,
412414

413415
absl::Status RegisterStringsFunctions(
414416
google::api::expr::runtime::CelFunctionRegistry* registry,
415-
const google::api::expr::runtime::InterpreterOptions& options) {
417+
const google::api::expr::runtime::InterpreterOptions& options,
418+
const StringsExtensionOptions& extension_options) {
416419
return RegisterStringsFunctions(
417420
registry->InternalGetRegistry(),
418-
google::api::expr::runtime::ConvertToRuntimeOptions(options));
421+
google::api::expr::runtime::ConvertToRuntimeOptions(options),
422+
extension_options);
419423
}
420424

421-
CheckerLibrary StringsCheckerLibrary(int version) {
425+
CheckerLibrary StringsCheckerLibrary(const StringsExtensionOptions& options) {
426+
const int version = options.version;
422427
return {"strings", [version](TypeCheckerBuilder& builder) {
423428
return RegisterStringsDecls(builder, version);
424429
}};

‎extensions/strings.h‎

Lines changed: 29 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -27,21 +27,45 @@ namespace cel::extensions {
2727

2828
constexpr int kStringsExtensionLatestVersion = 4;
2929

30+
struct StringsExtensionOptions {
31+
int version = kStringsExtensionLatestVersion;
32+
33+
// Maximum precision allowed for floating point format specifiers in
34+
// format() function. This is used for both fixed and scientific notations.
35+
// Value must be in the range [0, 1000], otherwise clamped.
36+
//
37+
// Does not affect default precisions for %e and %f format specifiers.
38+
int max_precision = 1000;
39+
};
40+
3041
// Register extension functions for strings.
3142
absl::Status RegisterStringsFunctions(
3243
FunctionRegistry& registry, const RuntimeOptions& options,
33-
int version = kStringsExtensionLatestVersion);
44+
const StringsExtensionOptions& extension_options = {});
3445

3546
absl::Status RegisterStringsFunctions(
3647
google::api::expr::runtime::CelFunctionRegistry* registry,
37-
const google::api::expr::runtime::InterpreterOptions& options);
48+
const google::api::expr::runtime::InterpreterOptions& options,
49+
const StringsExtensionOptions& extension_options = {});
3850

3951
CheckerLibrary StringsCheckerLibrary(
40-
int version = kStringsExtensionLatestVersion);
52+
const StringsExtensionOptions& extension_options = {});
53+
54+
inline CheckerLibrary StringsCheckerLibrary(int version) {
55+
StringsExtensionOptions options;
56+
options.version = version;
57+
return StringsCheckerLibrary(options);
58+
}
4159

4260
inline CompilerLibrary StringsCompilerLibrary(
43-
int version = kStringsExtensionLatestVersion) {
44-
return CompilerLibrary::FromCheckerLibrary(StringsCheckerLibrary(version));
61+
const StringsExtensionOptions& options = {}) {
62+
return CompilerLibrary::FromCheckerLibrary(StringsCheckerLibrary(options));
63+
}
64+
65+
inline CompilerLibrary StringsCompilerLibrary(int version) {
66+
StringsExtensionOptions options;
67+
options.version = version;
68+
return StringsCompilerLibrary(options);
4569
}
4670

4771
} // namespace cel::extensions

‎extensions/strings_test.cc‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ namespace cel::extensions {
5050
namespace {
5151

5252
using ::absl_testing::IsOk;
53+
using ::absl_testing::StatusIs;
5354
using ::cel::expr::ParsedExpr;
5455
using ::google::api::expr::parser::Parse;
5556
using ::google::api::expr::parser::ParserOptions;
@@ -85,6 +86,48 @@ TEST(StringsCheckerLibrary, SmokeTest) {
8586
)~bool^equals)");
8687
}
8788

89+
TEST(StringsExtTest, MaxPrecisionOption) {
90+
StringsExtensionOptions extension_options;
91+
extension_options.max_precision = 99;
92+
93+
ASSERT_OK_AND_ASSIGN(
94+
auto compiler_builder,
95+
NewCompilerBuilder(internal::GetTestingDescriptorPool()));
96+
97+
ASSERT_THAT(compiler_builder->AddLibrary(StandardCompilerLibrary()), IsOk());
98+
ASSERT_THAT(compiler_builder->AddLibrary(StringsCompilerLibrary()), IsOk());
99+
100+
ASSERT_OK_AND_ASSIGN(auto compiler, compiler_builder->Build());
101+
102+
auto result = compiler->Compile("'abc %.100f'.format([2.0])", "<input>");
103+
104+
ASSERT_THAT(result, IsOk());
105+
ASSERT_TRUE(result->IsValid());
106+
107+
RuntimeOptions opts;
108+
ASSERT_OK_AND_ASSIGN(
109+
auto runtime_builder,
110+
CreateStandardRuntimeBuilder(internal::GetTestingDescriptorPool(), opts));
111+
112+
ASSERT_THAT(RegisterStringsFunctions(runtime_builder.function_registry(),
113+
opts, extension_options),
114+
IsOk());
115+
116+
ASSERT_OK_AND_ASSIGN(auto runtime, std::move(runtime_builder).Build());
117+
118+
ASSERT_OK_AND_ASSIGN(auto program,
119+
runtime->CreateProgram(*result->ReleaseAst()));
120+
121+
google::protobuf::Arena arena;
122+
cel::Activation activation;
123+
ASSERT_OK_AND_ASSIGN(auto value, program->Evaluate(&arena, activation));
124+
125+
ASSERT_TRUE(value.Is<ErrorValue>());
126+
EXPECT_THAT(value.GetError().ToStatus(),
127+
StatusIs(absl::StatusCode::kInvalidArgument,
128+
HasSubstr("precision specifier exceeds maximum of 99")));
129+
}
130+
88131
using StringsExtFunctionsTest = testing::TestWithParam<std::string>;
89132

90133
TEST_P(StringsExtFunctionsTest, ParserAndCheckerTests) {

0 commit comments

Comments
 (0)