[regexp] Fix a possible off-by-one in Trace::cp_offset

schuay · V8 LUCI CQ · commit ddf1b5dac063 · 2025-10-14T00:25:06.000-07:00
.. which was itself guaranteed to be within kMinCPOffset and kMaxCPOffset. But some callers then went on to subtract 1, therefore exceeding kMinCPOffset. Ideally, callers would check that their updated value is still in bounds, and take appropriate action if not. But unfortunately, some current callers (e.g. in RegExpMacroAssembler) *have* no appropriate action, since they cannot abort. So this CL instead takes the approach of adding a kCPOffsetSlack instead, making the range checks in Trace stricter in order to allow caller "misbehavior". We also change cp_offset range DCHECKs into CHECKs since I'm not fully convinced that we correctly observe bounds everywhere. Fixed: 451663011 Change-Id: Ib50685d00a490a2959880bdd2fbeae5228a55997 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7037653 Reviewed-by: Patrick Thier <pthier@chromium.org> Commit-Queue: Jakob Linke <jgruber@chromium.org> Cr-Commit-Position: refs/heads/main@{#103102}
diff --git a/src/regexp/regexp-bytecode-generator.cc b/src/regexp/regexp-bytecode-generator.cc
@@ -204,8 +204,7 @@ void RegExpBytecodeGenerator::LoadCurrentCharacterImpl(int cp_offset,
     check_bounds = false;  // Load below doesn't need to check.
   }
 
-  DCHECK_LE(kMinCPOffset, cp_offset);
-  DCHECK_GE(kMaxCPOffset, cp_offset);
+  CHECK(base::IsInRange(cp_offset, kMinCPOffset, kMaxCPOffset));
   int bytecode;
   if (check_bounds) {
     if (characters == 4) {
diff --git a/src/regexp/regexp-compiler.cc b/src/regexp/regexp-compiler.cc
@@ -2313,6 +2313,7 @@ void AssertionNode::BacktrackIfPrevious(
   // If we've already checked that we are not at the start of input, it's okay
   // to load the previous character without bounds checks.
   const bool can_skip_bounds_check = !may_be_at_or_before_subject_string_start;
+  static_assert(Trace::kCPOffsetSlack == 1);
   assembler->LoadCurrentCharacter(new_trace.cp_offset() - 1, non_word,
                                   can_skip_bounds_check);
   EmitWordCheck(assembler, word, non_word, backtrack_if_previous == kIsNonWord);
@@ -2567,6 +2568,7 @@ void TextNode::Emit(RegExpCompiler* compiler, Trace* trace) {
   }
 
   bool first_elt_done = false;
+  static_assert(Trace::kCPOffsetSlack == 1);
   int bound_checked_to = trace->cp_offset() - 1;
   bound_checked_to += trace->bound_checked_up_to();
 
@@ -2611,7 +2613,10 @@ void Trace::AdvanceCurrentPositionInTrace(int by, RegExpCompiler* compiler) {
   // characters by means of mask and compare.
   quick_check_performed_.Advance(by, compiler->one_byte());
   cp_offset_ += by;
-  if (cp_offset_ > RegExpMacroAssembler::kMaxCPOffset) {
+  static_assert(RegExpMacroAssembler::kMaxCPOffset ==
+                -RegExpMacroAssembler::kMinCPOffset);
+  if (std::abs(cp_offset_) + kCPOffsetSlack >
+      RegExpMacroAssembler::kMaxCPOffset) {
     compiler->SetRegExpTooBig();
     cp_offset_ = 0;
   }
diff --git a/src/regexp/regexp-compiler.h b/src/regexp/regexp-compiler.h
@@ -278,7 +278,17 @@ class Trace {
   };
   void Flush(RegExpCompiler* compiler, RegExpNode* successor,
              FlushMode mode = kFlushFull);
+
+  // Some callers add/subtract 1 from cp_offset, assuming that the result is
+  // still valid. That's obviously not the case when our `cp_offset` is only
+  // checked against kMinCPOffset/kMaxCPOffset, so we need to apply the some
+  // slack.
+  // TODO(jgruber): It would be better if all callers checked against limits
+  // themselves when doing so; but unfortunately not all callers have
+  // abort-compilation mechanisms.
+  static constexpr int kCPOffsetSlack = 1;
   int cp_offset() const { return cp_offset_; }
+
   // Does any trace in the chain have an action?
   bool has_any_actions() const { return has_any_actions_; }
   // Does this particular trace object have an action?
diff --git a/src/regexp/regexp-macro-assembler.cc b/src/regexp/regexp-macro-assembler.cc
@@ -261,7 +261,7 @@ void NativeRegExpMacroAssembler::LoadCurrentCharacterImpl(
   // path requires a large number of characters, but not the reverse.
   DCHECK_GE(eats_at_least, characters);
 
-  DCHECK(base::IsInRange(cp_offset, kMinCPOffset, kMaxCPOffset));
+  CHECK(base::IsInRange(cp_offset, kMinCPOffset, kMaxCPOffset));
   if (check_bounds) {
     if (cp_offset >= 0) {
       CheckPosition(cp_offset + eats_at_least - 1, on_end_of_input);
diff --git a/test/mjsunit/regress/regress-451663011.js b/test/mjsunit/regress/regress-451663011.js
@@ -0,0 +1,9 @@
+// Copyright 2025 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+const length = 32767;
+const pattern_body = "^" + "a".repeat(length);
+const pattern = new RegExp("(?<=" + pattern_body + ")", "m");
+const input = "a".repeat(length) + "b" + '\n';
+assertThrows(() => pattern.exec(input));

Original file line number	Diff line number	Diff line change
`@@ -204,8 +204,7 @@ void RegExpBytecodeGenerator::LoadCurrentCharacterImpl(int cp_offset,`
`204`	`204`	`check_bounds = false; // Load below doesn't need to check.`
`205`	`205`	`}`
`206`	`206`
`207`		`- DCHECK_LE(kMinCPOffset, cp_offset);`
`208`		`- DCHECK_GE(kMaxCPOffset, cp_offset);`
	`207`	`+ CHECK(base::IsInRange(cp_offset, kMinCPOffset, kMaxCPOffset));`
`209`	`208`	`int bytecode;`
`210`	`209`	`if (check_bounds) {`
`211`	`210`	`if (characters == 4) {`