Skip to content

Commit c9fc788

Browse files
committed
Actions: Fix bad performance in getTargetPath
Seen on `github/codeql`, some queries had very poor performance: ``` [2/24 eval 36m4s] Evaluation done; writing results to codeql/actions-queries/Security/CWE-312/ExcessiveSecretsExposure.bqrs ``` Investigating further lead to the following worrying sequence of joins (after I ran out of patience and cancelled the query): ``` [2025-04-01 12:31:03] Tuple counts for Yaml::YamlInclude.getTargetPath/0#dispred#32565107#fb#reorder_1_0/2@i6#9f4b2jw1 after 8m40s: ... 559418 ~33% {1} r5 = SCAN `Yaml::YamlNode.getLocation/0#dispred#24555c57#prev_delta` OUTPUT In.1 ... 909345525 ~821% {3} r7 = JOIN r5 WITH `Yaml::YamlNode.getLocation/0#dispred#24555c57#prev` CARTESIAN PRODUCT OUTPUT Rhs.1, Lhs.0 'result', Rhs.0 909342139 ~779% {3} | JOIN WITH `Locations::Location.getFile/0#dispred#dcf38c8d#prev` ON FIRST 1 OUTPUT Rhs.1, Lhs.1 'result', Lhs.2 909338753 ~794% {3} | JOIN WITH containerparent_10#join_rhs ON FIRST 1 OUTPUT Rhs.1, Lhs.1 'result', Lhs.2 909335367 ~824% {3} | JOIN WITH `FileSystem::Container.getAbsolutePath/0#dispred#d234e6fa` ON FIRST 1 OUTPUT Lhs.2, Lhs.1 'result', Rhs.1 883246724 ~812% {3} | JOIN WITH `Yaml::YamlNode.getDocument/0#dispred#ee1eb3bf#bf_10#join_rhs` ON FIRST 1 OUTPUT Rhs.1 'this', Lhs.1 'result', Lhs.2 760047185 ~838% {5} | JOIN WITH yaml_scalars ON FIRST 1 OUTPUT Lhs.1 'result', Lhs.0 'this', Rhs.2, _, Lhs.2 0 ~0% {4} | REWRITE WITH Tmp.3 := "/", Out.3 := (In.4 ++ Tmp.3 ++ InOut.2), TEST Out.3 = InOut.0 KEEPING 4 {4} | REWRITE WITH NOT [TEST InOut.2 startsWith "/"] ... ``` The culprit turned out to be the following method on class `YamlInclude` ```ql private string getTargetPath() { exists(string path | path = this.getValue() | if path.matches("/%") then result = path else result = this.getDocument().getLocation().getFile().getParentContainer().getAbsolutePath() + "/" + path ) } ``` Basically, in the `else` branch, the evaluator was producing all possible values of `result` before filtering out the ones where the `path` component started with a forward slash. To fix this, I opted to factor out the logic into two helper predicates, each accounting for whether `this.getValue()` does or does not start with a `/`. With this, evaluating the original query from a clean cache takes roughly 3.3s.
1 parent ffb25b7 commit c9fc788

File tree

1 file changed

+17
-8
lines changed

1 file changed

+17
-8
lines changed

shared/yaml/codeql/yaml/Yaml.qll

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -424,14 +424,23 @@ module Make<InputSig Input> {
424424
* Gets the absolute path of the file included by this directive.
425425
*/
426426
private string getTargetPath() {
427-
exists(string path | path = this.getValue() |
428-
if path.matches("/%")
429-
then result = path
430-
else
431-
result =
432-
this.getDocument().getLocation().getFile().getParentContainer().getAbsolutePath() + "/" +
433-
path
434-
)
427+
result = this.getAbsolutePath()
428+
or
429+
result =
430+
this.getDocument().getLocation().getFile().getParentContainer().getAbsolutePath() + "/" +
431+
this.getRelativePath()
432+
}
433+
434+
/** Join-order helper for `getTargetPath`. Gets the path but only if it is an absolute path. */
435+
private string getAbsolutePath() {
436+
result = this.getValue() and
437+
result.matches("/%")
438+
}
439+
440+
/** Join-order helper for `getTargetPath`. Gets the path, but only if it is a relative path. */
441+
private string getRelativePath() {
442+
result = this.getValue() and
443+
not result.matches("/%")
435444
}
436445
}
437446

0 commit comments

Comments
 (0)