Skip to content

Commit 70d76cc

Browse files
felickzCopilot
andcommitted
Eliminate redundant per-page-class join in isBracketedForTagHelperAttribute
The shared 'page' variable forced the evaluator to redundantly repeat the expensive same-basic-block join once per RazorPageBase-derived class (e.g. 525 classes in one real-world repo) instead of once per distinct shared WriteLiteral/Begin/EndWriteTagHelperAttribute Method. Resolving each call independently is semantically equivalent (the per-instance correlation is already guaranteed by hasImplicitThisQualifier() + same BasicBlock) and removes an O(page-classes x calls) blowup that caused unbounded runtime on Razor-heavy codebases. Validated: direct DB runs against 3 real repos (before/after result counts unchanged, cache-cleared A/B equivalence test), plus MRVA runs on 2-core (45+min hang -> 4m22s) and 4-core (3m35s) GitHub Actions runners for the same repo batch. No change note: this predicate was introduced by this same unmerged PR and has never shipped, so this is pre-release refinement, not a behavior/perf fix to released code. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 463cbe9c-9db5-425f-8784-63ea319314b9
1 parent befe578 commit 70d76cc

1 file changed

Lines changed: 32 additions & 15 deletions

File tree

  • csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks

‎csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/Html.qll‎

Lines changed: 32 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -180,7 +180,7 @@ class MicrosoftAspNetCoreMvcHtmlHelperRawSink extends AspNetCoreHtmlSink {
180180
/**
181181
* Holds if `writeLiteral` is a call to `RazorPageBase.WriteLiteral` whose argument is captured
182182
* between a matching pair of `BeginWriteTagHelperAttribute()`/`EndWriteTagHelperAttribute()`
183-
* calls on `page`, in the same basic block, with no other such calls in between.
183+
* calls, in the same basic block, with no other such calls in between.
184184
*
185185
* The Razor source generator emits this bracketing for every literal or expression segment of an
186186
* HTML attribute value on an element that also carries a tag helper (for example `asp-for`). Such
@@ -191,28 +191,39 @@ class MicrosoftAspNetCoreMvcHtmlHelperRawSink extends AspNetCoreHtmlSink {
191191
*
192192
* Because a basic block cannot contain a branch, requiring `beginCall`, `writeLiteral`, and
193193
* `endCall` to appear (in that order) in the same basic block, with no other
194-
* `Begin`/`EndWriteTagHelperAttribute` call from `page` strictly between `beginCall` and
195-
* `endCall`, guarantees that `beginCall`/`endCall` are the immediately enclosing bracket around
194+
* `Begin`/`EndWriteTagHelperAttribute` call strictly between `beginCall` and `endCall`,
195+
* guarantees that `beginCall`/`endCall` are the immediately enclosing bracket around
196196
* `writeLiteral` on every path that reaches it (that is, the bracket opened by `beginCall` is
197197
* still open, and not yet closed by some other `endCall`, at the point `writeLiteral` executes).
198198
* No other such call can coincide with `writeLiteral` itself, so checking the whole open interval
199199
* between `beginCall` and `endCall` is equivalent to checking it on both sides of `writeLiteral`
200200
* separately.
201201
*
202202
* `beginCall`, `writeLiteral`, and `endCall` are additionally required to have an implicit `this`
203-
* qualifier, which is how the Razor source generator always emits these calls. This ensures all
204-
* three calls act on the same page instance, so a bracket on one page cannot be mistaken for a
205-
* bracket around a `WriteLiteral` call on a different page.
203+
* qualifier, which is how the Razor source generator always emits these calls. Combined with all
204+
* three calls being required to lie in the same basic block (and hence the same method body),
205+
* this ensures all three calls act on the same page instance, so a bracket on one page cannot be
206+
* mistaken for a bracket around a `WriteLiteral` call on a different page.
207+
*
208+
* The `WriteLiteral`/`BeginWriteTagHelperAttribute`/`EndWriteTagHelperAttribute` methods are
209+
* looked up via `any(MicrosoftAspNetCoreMvcRazorPageBase page).get...Method()` rather than
210+
* through a single shared `page` variable bound across `writeLiteral`, `beginCall`, and
211+
* `endCall`. These methods are inherited (not overridden) from the shared `RazorPageBase`
212+
* framework type, so every generated Razor page class resolves to the same handful of `Method`
213+
* entities; binding a single `page` variable across all three calls would force the join to be
214+
* repeated once per generated page class in the codebase, rather than once per distinct `Method`,
215+
* causing severe performance degradation on codebases with many Razor views.
206216
*/
207217
private predicate isBracketedForTagHelperAttribute(Call writeLiteral) {
208-
exists(
209-
MicrosoftAspNetCoreMvcRazorPageBase page, MethodCall beginCall, MethodCall endCall,
210-
BasicBlock bb, int i, int j, int k
211-
|
218+
exists(MethodCall beginCall, MethodCall endCall, BasicBlock bb, int i, int j, int k |
212219
bb = writeLiteral.getBasicBlock() and
213-
writeLiteral = page.getWriteLiteralMethod().getACall() and
214-
beginCall = page.getBeginWriteTagHelperAttributeMethod().getACall() and
215-
endCall = page.getEndWriteTagHelperAttributeMethod().getACall() and
220+
writeLiteral = any(MicrosoftAspNetCoreMvcRazorPageBase page).getWriteLiteralMethod().getACall() and
221+
beginCall =
222+
any(MicrosoftAspNetCoreMvcRazorPageBase page)
223+
.getBeginWriteTagHelperAttributeMethod()
224+
.getACall() and
225+
endCall =
226+
any(MicrosoftAspNetCoreMvcRazorPageBase page).getEndWriteTagHelperAttributeMethod().getACall() and
216227
writeLiteral.(QualifiableExpr).hasImplicitThisQualifier() and
217228
beginCall.hasImplicitThisQualifier() and
218229
endCall.hasImplicitThisQualifier() and
@@ -223,8 +234,14 @@ private predicate isBracketedForTagHelperAttribute(Call writeLiteral) {
223234
j < k and
224235
not exists(int l, Call other |
225236
(
226-
other = page.getBeginWriteTagHelperAttributeMethod().getACall() or
227-
other = page.getEndWriteTagHelperAttributeMethod().getACall()
237+
other =
238+
any(MicrosoftAspNetCoreMvcRazorPageBase page)
239+
.getBeginWriteTagHelperAttributeMethod()
240+
.getACall() or
241+
other =
242+
any(MicrosoftAspNetCoreMvcRazorPageBase page)
243+
.getEndWriteTagHelperAttributeMethod()
244+
.getACall()
228245
) and
229246
bb.getNode(l) = other.getControlFlowNode() and
230247
i < l and

0 commit comments

Comments
 (0)