From c50212e96924a1911cd03f29096f663731d5e890 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Wed, 11 Feb 2026 16:07:43 +0100 Subject: [PATCH] Rust: Make path resolution robust against invalid code with conflicting declarations --- .../codeql/rust/internal/PathResolution.qll | 60 ++++++++++++------- .../path-resolution/invalid/main.rs | 6 ++ .../path-resolution/invalid/options.yml | 1 + .../path-resolution/path-resolution.expected | 1 + 4 files changed, 45 insertions(+), 23 deletions(-) create mode 100644 rust/ql/test/library-tests/path-resolution/invalid/main.rs create mode 100644 rust/ql/test/library-tests/path-resolution/invalid/options.yml diff --git a/rust/ql/lib/codeql/rust/internal/PathResolution.qll b/rust/ql/lib/codeql/rust/internal/PathResolution.qll index 432608f6f9d6..893801a4a580 100644 --- a/rust/ql/lib/codeql/rust/internal/PathResolution.qll +++ b/rust/ql/lib/codeql/rust/internal/PathResolution.qll @@ -107,7 +107,7 @@ class SuccessorKind extends TSuccessorKind { } pragma[nomagic] -private ItemNode getAChildSuccessor(ItemNode item, string name, SuccessorKind kind) { +private ItemNode getAChildSuccessor0(ItemNode item, string name, SuccessorKind kind) { item = result.getImmediateParent() and name = result.getName() and // Associated items in `impl` and `trait` blocks are handled elsewhere @@ -116,7 +116,7 @@ private ItemNode getAChildSuccessor(ItemNode item, string name, SuccessorKind ki if result instanceof TypeParam then kind.isInternal() else - if result.isPublic() + if result.isPublic() or item instanceof SourceFile then kind.isBoth() else kind.isInternal() or @@ -130,6 +130,28 @@ private ItemNode getAChildSuccessor(ItemNode item, string name, SuccessorKind ki result = item } +pragma[nomagic] +private ItemNode getAChildSuccessor(ItemNode item, string name, SuccessorKind kind) { + result = getAChildSuccessor0(item, name, kind) and + ( + not result instanceof NamedItemNode + or + // In valid Rust code, there cannot be multiple children with the same name and namespace, + // but to safe-guard against combinatorial explosions in invalid code, we always pick the + // last child + exists(Namespace ns | + result = + max(NamedItemNode i, int startline, int startcolumn, int endline, int endcolumn | + i.getLocation().hasLocationInfo(_, startline, startcolumn, endline, endcolumn) and + i = getAChildSuccessor0(item, name, _) and + ns = i.getNamespace() + | + i order by startline, startcolumn, endline, endcolumn + ) + ) + ) +} + private module UseOption = Option; private class UseOption = UseOption::Option; @@ -288,10 +310,6 @@ abstract class ItemNode extends Locatable { cached ItemNode getASuccessor(string name, SuccessorKind kind, UseOption useOpt) { Stages::PathResolutionStage::ref() and - sourceFileEdge(this, name, result) and - kind.isBoth() and - useOpt.isNone() - or result = getAChildSuccessor(this, name, kind) and useOpt.isNone() or @@ -471,6 +489,8 @@ abstract class ItemNode extends Locatable { Location getLocation() { result = super.getLocation() } } +abstract class NamedItemNode extends ItemNode { } + abstract class TypeItemNode extends ItemNode { } /** A module or a source file. */ @@ -509,7 +529,7 @@ private class SourceFileItemNode extends ModuleLikeNode instanceof SourceFile { override string getCanonicalPath(Crate c) { none() } } -class CrateItemNode extends ItemNode instanceof Crate { +class CrateItemNode extends NamedItemNode instanceof Crate { /** * Gets the source file that defines this crate. */ @@ -565,7 +585,7 @@ class CrateItemNode extends ItemNode instanceof Crate { override string getCanonicalPath(Crate c) { c = this and result = Crate.super.getName() } } -class ExternCrateItemNode extends ItemNode instanceof ExternCrate { +class ExternCrateItemNode extends NamedItemNode instanceof ExternCrate { override string getName() { result = super.getRename().getName().getText() or @@ -573,7 +593,7 @@ class ExternCrateItemNode extends ItemNode instanceof ExternCrate { result = super.getIdentifier().getText() } - override Namespace getNamespace() { none() } + override Namespace getNamespace() { result.isType() } override Visibility getVisibility() { result = ExternCrate.super.getVisibility() } @@ -587,7 +607,7 @@ class ExternCrateItemNode extends ItemNode instanceof ExternCrate { } /** An item that can occur in a trait or an `impl` block. */ -abstract private class AssocItemNode extends ItemNode instanceof AssocItem { +abstract private class AssocItemNode extends NamedItemNode instanceof AssocItem { /** Holds if this associated item has an implementation. */ abstract predicate hasImplementation(); @@ -626,7 +646,7 @@ private class ConstItemNode extends AssocItemNode instanceof Const { override TypeParam getTypeParam(int i) { none() } } -private class TypeItemTypeItemNode extends TypeItemNode instanceof TypeItem { +private class TypeItemTypeItemNode extends NamedItemNode, TypeItemNode instanceof TypeItem { override string getName() { result = TypeItem.super.getName().getText() } override Namespace getNamespace() { result.isType() } @@ -659,7 +679,7 @@ private class TypeItemTypeItemNode extends TypeItemNode instanceof TypeItem { } /** An item that can be referenced with arguments. */ -abstract class ParameterizableItemNode extends ItemNode { +abstract class ParameterizableItemNode extends NamedItemNode { /** Gets the arity this item. */ abstract int getArity(); } @@ -911,7 +931,7 @@ private class ImplTraitTypeReprItemNodeImpl extends ImplTraitTypeReprItemNode { ItemNode resolveABoundCand() { result = resolvePathCand(this.getABoundPath()) } } -private class ModuleItemNode extends ModuleLikeNode instanceof Module { +private class ModuleItemNode extends NamedItemNode, ModuleLikeNode instanceof Module { override string getName() { result = Module.super.getName().getText() } override Namespace getNamespace() { result.isType() } @@ -929,7 +949,7 @@ private class ModuleItemNode extends ModuleLikeNode instanceof Module { ( exists(SourceFile f | fileImport(this, f) and - sourceFileEdge(f, _, child) + child = getAChildSuccessor(f, _, _) ) or this = child.getImmediateParent() @@ -1001,7 +1021,7 @@ private class StructItemNode extends TypeItemTypeItemNode, ParameterizableItemNo } } -final class TraitItemNode extends ImplOrTraitItemNode, TypeItemNode instanceof Trait { +final class TraitItemNode extends ImplOrTraitItemNode, NamedItemNode, TypeItemNode instanceof Trait { pragma[nomagic] Path getABoundPath() { result = super.getATypeBound().getTypeRepr().(PathTypeRepr).getPath() } @@ -1126,7 +1146,7 @@ private class BlockExprItemNode extends ItemNode instanceof BlockExpr { pragma[nomagic] private Path getWherePredPath(WherePred wp) { result = wp.getTypeRepr().(PathTypeRepr).getPath() } -final class TypeParamItemNode extends TypeItemNode instanceof TypeParam { +final class TypeParamItemNode extends NamedItemNode, TypeItemNode instanceof TypeParam { /** Gets a where predicate for this type parameter, if any */ pragma[nomagic] private WherePred getAWherePred() { @@ -1214,7 +1234,7 @@ final private class TypeParamItemNodeImpl extends TypeParamItemNode instanceof T ItemNode resolveABoundCand() { result = resolvePathCand(this.getABoundPathCand()) } } -abstract private class MacroItemNode extends ItemNode { +abstract private class MacroItemNode extends NamedItemNode { override Namespace getNamespace() { result.isMacro() } override TypeParam getTypeParam(int i) { none() } @@ -1256,12 +1276,6 @@ private class MacroDefItemNode extends MacroItemNode instanceof MacroDef { override Attr getAnAttr() { result = MacroDef.super.getAnAttr() } } -/** Holds if `item` has the name `name` and is a top-level item inside `f`. */ -private predicate sourceFileEdge(SourceFile f, string name, ItemNode item) { - item = f.(ItemNode).getADescendant() and - name = item.getName() -} - /** Holds if `f` is available as `mod name;` inside `folder`. */ pragma[nomagic] private predicate fileModule(SourceFile f, string name, Folder folder) { diff --git a/rust/ql/test/library-tests/path-resolution/invalid/main.rs b/rust/ql/test/library-tests/path-resolution/invalid/main.rs new file mode 100644 index 000000000000..81e3913f1f60 --- /dev/null +++ b/rust/ql/test/library-tests/path-resolution/invalid/main.rs @@ -0,0 +1,6 @@ +// The code in this file is not valid Rust code + +struct A; // A1 +struct A; // A2 + +fn f(x: A) {} // $ item=A2 (the later occurence takes precendence) diff --git a/rust/ql/test/library-tests/path-resolution/invalid/options.yml b/rust/ql/test/library-tests/path-resolution/invalid/options.yml new file mode 100644 index 000000000000..cf148dd35f86 --- /dev/null +++ b/rust/ql/test/library-tests/path-resolution/invalid/options.yml @@ -0,0 +1 @@ +qltest_cargo_check: false diff --git a/rust/ql/test/library-tests/path-resolution/path-resolution.expected b/rust/ql/test/library-tests/path-resolution/path-resolution.expected index 153d80db4cca..e85bb7876dab 100644 --- a/rust/ql/test/library-tests/path-resolution/path-resolution.expected +++ b/rust/ql/test/library-tests/path-resolution/path-resolution.expected @@ -51,6 +51,7 @@ mod | my/nested.rs:1:1:17:1 | mod nested1 | | my/nested.rs:2:5:11:5 | mod nested2 | resolvePath +| invalid/main.rs:6:9:6:9 | A | invalid/main.rs:3:11:4:9 | struct A | | main.rs:4:8:4:9 | my | main.rs:1:1:1:7 | mod my | | main.rs:4:14:4:17 | self | main.rs:1:1:1:7 | mod my | | main.rs:6:5:6:6 | my | main.rs:1:1:1:7 | mod my |