hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-04-29 18:55:14 +02:00
Code Issues Packages Projects Releases Wiki Activity

Merge branch 'main' into experimental-decompression-api

Browse Source
This commit is contained in:
thiggy1342
2022-06-17 12:30:38 -04:00
committed by GitHub
parent 4163ae1ca3 a7993fb455
commit b171883cd0
38 changed files with 873 additions and 289 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
go/extractor/cli/go-autobuilder/go-autobuilder.go
View File

@@ -291,7 +291,7 @@ func main() {
}
// Go 1.16 and later won't automatically attempt to update go.mod / go.sum during package loading, so try to update them here:
if depMode == GoGetWithModules && semver.Compare(getEnvGoSemVer(), "1.16") >= 0 {
if modMode != ModVendor && depMode == GoGetWithModules && semver.Compare(getEnvGoSemVer(), "1.16") >= 0 {
// stat go.mod and go.sum
beforeGoModFileInfo, beforeGoModErr := os.Stat("go.mod")
if beforeGoModErr != nil {

74
java/kotlin-extractor/src/main/kotlin/KotlinUsesExtractor.kt
View File

@@ -66,80 +66,6 @@ open class KotlinUsesExtractor(
TypeResult(fakeKotlinType(), "", "")
)
private data class MethodKey(val className: FqName, val functionName: Name)
private fun makeDescription(className: FqName, functionName: String) = MethodKey(className, Name.guessByFirstCharacter(functionName))
// This essentially mirrors SpecialBridgeMethods.kt, a backend pass which isn't easily available to our extractor.
private val specialFunctions = mapOf(
makeDescription(StandardNames.FqNames.collection, "<get-size>") to "size",
makeDescription(FqName("java.util.Collection"), "<get-size>") to "size",
makeDescription(StandardNames.FqNames.map, "<get-size>") to "size",
makeDescription(FqName("java.util.Map"), "<get-size>") to "size",
makeDescription(StandardNames.FqNames.charSequence.toSafe(), "<get-length>") to "length",
makeDescription(FqName("java.lang.CharSequence"), "<get-length>") to "length",
makeDescription(StandardNames.FqNames.map, "<get-keys>") to "keySet",
makeDescription(FqName("java.util.Map"), "<get-keys>") to "keySet",
makeDescription(StandardNames.FqNames.map, "<get-values>") to "values",
makeDescription(FqName("java.util.Map"), "<get-values>") to "values",
makeDescription(StandardNames.FqNames.map, "<get-entries>") to "entrySet",
makeDescription(FqName("java.util.Map"), "<get-entries>") to "entrySet",
makeDescription(StandardNames.FqNames.mutableList, "removeAt") to "remove",
makeDescription(FqName("java.util.List"), "removeAt") to "remove",
makeDescription(StandardNames.FqNames._enum.toSafe(), "<get-ordinal>") to "ordinal",
makeDescription(FqName("java.lang.Enum"), "<get-ordinal>") to "ordinal",
makeDescription(StandardNames.FqNames._enum.toSafe(), "<get-name>") to "name",
makeDescription(FqName("java.lang.Enum"), "<get-name>") to "name",
makeDescription(StandardNames.FqNames.number.toSafe(), "toByte") to "byteValue",
makeDescription(FqName("java.lang.Number"), "toByte") to "byteValue",
makeDescription(StandardNames.FqNames.number.toSafe(), "toShort") to "shortValue",
makeDescription(FqName("java.lang.Number"), "toShort") to "shortValue",
makeDescription(StandardNames.FqNames.number.toSafe(), "toInt") to "intValue",
makeDescription(FqName("java.lang.Number"), "toInt") to "intValue",
makeDescription(StandardNames.FqNames.number.toSafe(), "toLong") to "longValue",
makeDescription(FqName("java.lang.Number"), "toLong") to "longValue",
makeDescription(StandardNames.FqNames.number.toSafe(), "toFloat") to "floatValue",
makeDescription(FqName("java.lang.Number"), "toFloat") to "floatValue",
makeDescription(StandardNames.FqNames.number.toSafe(), "toDouble") to "doubleValue",
makeDescription(FqName("java.lang.Number"), "toDouble") to "doubleValue",
)
private val specialFunctionShortNames = specialFunctions.keys.map { it.functionName }.toSet()
fun getSpecialJvmName(f: IrFunction): String? {
if (specialFunctionShortNames.contains(f.name) && f is IrSimpleFunction) {
f.allOverridden(true).forEach { overriddenFunc ->
overriddenFunc.parentClassOrNull?.fqNameWhenAvailable?.let { parentFqName ->
specialFunctions[MethodKey(parentFqName, f.name)]?.let {
return it
}
}
}
}
return null
}
fun getJvmName(container: IrAnnotationContainer): String? {
for(a: IrConstructorCall in container.annotations) {
val t = a.type
if (t is IrSimpleType && a.valueArgumentsCount == 1) {
val owner = t.classifier.owner
val v = a.getValueArgument(0)
if (owner is IrClass) {
val aPkg = owner.packageFqName?.asString()
val name = owner.name.asString()
if(aPkg == "kotlin.jvm" && name == "JvmName" && v is IrConst<*>) {
val value = v.value
if (value is String) {
return value
}
}
}
}
}
return (container as? IrFunction)?.let { getSpecialJvmName(container) }
}
@OptIn(kotlin.ExperimentalStdlibApi::class) // Annotation required by kotlin versions < 1.5
fun extractFileClass(f: IrFile): Label<out DbClass> {
val fileName = f.fileEntry.name

9
java/kotlin-extractor/src/main/kotlin/utils/ClassNames.kt
View File

@@ -1,5 +1,6 @@
package com.github.codeql
import com.github.codeql.utils.getJvmName
import com.intellij.openapi.vfs.StandardFileSystems
import org.jetbrains.kotlin.load.java.sources.JavaSourceElement
import org.jetbrains.kotlin.load.java.structure.impl.classFiles.BinaryJavaClass
@@ -18,17 +19,19 @@ import org.jetbrains.kotlin.load.kotlin.JvmPackagePartSource
// and declarations within them into the parent class' JLS 13.1 name as
// specified above, followed by a `$` separator and then the short name
// for `that`.
private fun getName(d: IrDeclarationWithName) = (d as? IrAnnotationContainer)?.let { getJvmName(it) } ?: d.name.asString()
fun getIrDeclBinaryName(that: IrDeclaration): String {
val shortName = when(that) {
is IrDeclarationWithName -> that.name.asString()
is IrDeclarationWithName -> getName(that)
else -> "(unknown-name)"
}
val internalName = StringBuilder(shortName);
generateSequence(that.parent) { (it as? IrDeclaration)?.parent }
.forEach {
when (it) {
is IrClass -> internalName.insert(0, it.name.asString() + "$")
is IrPackageFragment -> it.fqName.asString().takeIf { it.isNotEmpty() }?.let { internalName.insert(0, "$it.") }
is IrClass -> internalName.insert(0, getName(it) + "$")
is IrPackageFragment -> it.fqName.asString().takeIf { fqName -> fqName.isNotEmpty() }?.let { fqName -> internalName.insert(0, "$fqName.") }
}
}
return internalName.toString()

90
java/kotlin-extractor/src/main/kotlin/utils/JvmNames.kt Normal file
View File

@@ -0,0 +1,90 @@
package com.github.codeql.utils
import org.jetbrains.kotlin.backend.common.ir.allOverridden
import org.jetbrains.kotlin.builtins.StandardNames
import org.jetbrains.kotlin.ir.declarations.IrAnnotationContainer
import org.jetbrains.kotlin.ir.declarations.IrClass
import org.jetbrains.kotlin.ir.declarations.IrFunction
import org.jetbrains.kotlin.ir.declarations.IrSimpleFunction
import org.jetbrains.kotlin.ir.expressions.IrConst
import org.jetbrains.kotlin.ir.expressions.IrConstructorCall
import org.jetbrains.kotlin.ir.types.IrSimpleType
import org.jetbrains.kotlin.ir.util.fqNameWhenAvailable
import org.jetbrains.kotlin.ir.util.packageFqName
import org.jetbrains.kotlin.ir.util.parentClassOrNull
import org.jetbrains.kotlin.name.FqName
import org.jetbrains.kotlin.name.Name
private data class MethodKey(val className: FqName, val functionName: Name)
private fun makeDescription(className: FqName, functionName: String) = MethodKey(className, Name.guessByFirstCharacter(functionName))
// This essentially mirrors SpecialBridgeMethods.kt, a backend pass which isn't easily available to our extractor.
private val specialFunctions = mapOf(
makeDescription(StandardNames.FqNames.collection, "<get-size>") to "size",
makeDescription(FqName("java.util.Collection"), "<get-size>") to "size",
makeDescription(StandardNames.FqNames.map, "<get-size>") to "size",
makeDescription(FqName("java.util.Map"), "<get-size>") to "size",
makeDescription(StandardNames.FqNames.charSequence.toSafe(), "<get-length>") to "length",
makeDescription(FqName("java.lang.CharSequence"), "<get-length>") to "length",
makeDescription(StandardNames.FqNames.map, "<get-keys>") to "keySet",
makeDescription(FqName("java.util.Map"), "<get-keys>") to "keySet",
makeDescription(StandardNames.FqNames.map, "<get-values>") to "values",
makeDescription(FqName("java.util.Map"), "<get-values>") to "values",
makeDescription(StandardNames.FqNames.map, "<get-entries>") to "entrySet",
makeDescription(FqName("java.util.Map"), "<get-entries>") to "entrySet",
makeDescription(StandardNames.FqNames.mutableList, "removeAt") to "remove",
makeDescription(FqName("java.util.List"), "removeAt") to "remove",
makeDescription(StandardNames.FqNames._enum.toSafe(), "<get-ordinal>") to "ordinal",
makeDescription(FqName("java.lang.Enum"), "<get-ordinal>") to "ordinal",
makeDescription(StandardNames.FqNames._enum.toSafe(), "<get-name>") to "name",
makeDescription(FqName("java.lang.Enum"), "<get-name>") to "name",
makeDescription(StandardNames.FqNames.number.toSafe(), "toByte") to "byteValue",
makeDescription(FqName("java.lang.Number"), "toByte") to "byteValue",
makeDescription(StandardNames.FqNames.number.toSafe(), "toShort") to "shortValue",
makeDescription(FqName("java.lang.Number"), "toShort") to "shortValue",
makeDescription(StandardNames.FqNames.number.toSafe(), "toInt") to "intValue",
makeDescription(FqName("java.lang.Number"), "toInt") to "intValue",
makeDescription(StandardNames.FqNames.number.toSafe(), "toLong") to "longValue",
makeDescription(FqName("java.lang.Number"), "toLong") to "longValue",
makeDescription(StandardNames.FqNames.number.toSafe(), "toFloat") to "floatValue",
makeDescription(FqName("java.lang.Number"), "toFloat") to "floatValue",
makeDescription(StandardNames.FqNames.number.toSafe(), "toDouble") to "doubleValue",
makeDescription(FqName("java.lang.Number"), "toDouble") to "doubleValue",
)
private val specialFunctionShortNames = specialFunctions.keys.map { it.functionName }.toSet()
fun getSpecialJvmName(f: IrFunction): String? {
if (specialFunctionShortNames.contains(f.name) && f is IrSimpleFunction) {
f.allOverridden(true).forEach { overriddenFunc ->
overriddenFunc.parentClassOrNull?.fqNameWhenAvailable?.let { parentFqName ->
specialFunctions[MethodKey(parentFqName, f.name)]?.let {
return it
}
}
}
}
return null
}
fun getJvmName(container: IrAnnotationContainer): String? {
for(a: IrConstructorCall in container.annotations) {
val t = a.type
if (t is IrSimpleType && a.valueArgumentsCount == 1) {
val owner = t.classifier.owner
val v = a.getValueArgument(0)
if (owner is IrClass) {
val aPkg = owner.packageFqName?.asString()
val name = owner.name.asString()
if(aPkg == "kotlin.jvm" && name == "JvmName" && v is IrConst<*>) {
val value = v.value
if (value is String) {
return value
}
}
}
}
}
return (container as? IrFunction)?.let { getSpecialJvmName(container) }
}

4
java/ql/src/Likely Bugs/Cloning/MissingCallToSuperClone.ql
View File

@@ -20,5 +20,5 @@ where
c.fromSource() and
exists(sc.getBody()) and
not exists(CloneMethod ssc | sc.callsSuper(ssc))
select sc, "This clone method does not call super.clone(), but is " + "overridden and called $@.",
c, "in a subclass"
select sc, "This clone method does not call super.clone(), but is overridden and called $@.", c,
"in a subclass"

2
java/ql/src/Likely Bugs/Likely Typos/ConstructorTypo.ql
View File

@@ -17,4 +17,4 @@ from Method m
where
m.hasName(m.getDeclaringType().getName()) and
m.fromSource()
select m, "This method has the same name as its declaring class." + " Should it be a constructor?"
select m, "This method has the same name as its declaring class. Should it be a constructor?"

3
java/ql/src/Likely Bugs/Serialization/NonSerializableField.ql
View File

@@ -96,5 +96,4 @@ where
not exceptions(c, f) and
reason = nonSerialReason(f.getType())
select f,
"This field is in a serializable class, " + " but is not serializable itself because " + reason +
"."
"This field is in a serializable class, but is not serializable itself because " + reason + "."

2
java/ql/test/kotlin/library-tests/maps-iterator-overloads/test.expected Normal file
View File

@@ -0,0 +1,2 @@
| test.kt:1:100:1:109 | mutableIterator(...) | mutableIterator |
| test.kt:3:73:3:82 | iterator(...) | iterator |

3
java/ql/test/kotlin/library-tests/maps-iterator-overloads/test.kt Normal file
View File

@@ -0,0 +1,3 @@
fun getIter(m: MutableMap<String, Int>): MutableIterator<MutableMap.MutableEntry<String, Int>> = m.iterator()
fun getIter2(m: Map<String, Int>): Iterator<Map.Entry<String, Int>> = m.iterator()

4
java/ql/test/kotlin/library-tests/maps-iterator-overloads/test.ql Normal file
View File

@@ -0,0 +1,4 @@
import java
from MethodAccess ma
select ma, ma.getCallee().toString()

36
java/ql/test/query-tests/NonSerializableField/NonSerializableField.expected
View File

@@ -1,18 +1,18 @@
| NonSerializableFieldTest.java:25:8:25:19 | problematic1 | This field is in a serializable class, but is not serializable itself because NS is not serializable. |
| NonSerializableFieldTest.java:26:14:26:25 | problematic2 | This field is in a serializable class, but is not serializable itself because NS is not serializable. |
| NonSerializableFieldTest.java:27:16:27:27 | problematic3 | This field is in a serializable class, but is not serializable itself because ? is not serializable. |
| NonSerializableFieldTest.java:27:16:27:27 | problematic3 | This field is in a serializable class, but is not serializable itself because NS is not serializable. |
| NonSerializableFieldTest.java:28:16:28:27 | problematic4 | This field is in a serializable class, but is not serializable itself because ? is not serializable. |
| NonSerializableFieldTest.java:28:16:28:27 | problematic4 | This field is in a serializable class, but is not serializable itself because NS is not serializable. |
| NonSerializableFieldTest.java:29:30:29:41 | problematic5 | This field is in a serializable class, but is not serializable itself because Map<?,NS> is not serializable. |
| NonSerializableFieldTest.java:30:9:30:20 | problematic6 | This field is in a serializable class, but is not serializable itself because Map<> is not serializable. |
| NonSerializableFieldTest.java:31:24:31:35 | problematic7 | This field is in a serializable class, but is not serializable itself because ? extends NS is not serializable. |
| NonSerializableFieldTest.java:32:22:32:33 | problematic8 | This field is in a serializable class, but is not serializable itself because ? super NS is not serializable. |
| NonSerializableFieldTest.java:33:7:33:18 | problematic9 | This field is in a serializable class, but is not serializable itself because T is not serializable. |
| NonSerializableFieldTest.java:34:13:34:25 | problematic10 | This field is in a serializable class, but is not serializable itself because T is not serializable. |
| NonSerializableFieldTest.java:35:13:35:25 | problematic11 | This field is in a serializable class, but is not serializable itself because ? is not serializable. |
| NonSerializableFieldTest.java:36:15:36:27 | problematic12 | This field is in a serializable class, but is not serializable itself because ? is not serializable. |
| NonSerializableFieldTest.java:37:34:37:46 | problematic13 | This field is in a serializable class, but is not serializable itself because Map<?,Double> is not serializable. |
| NonSerializableFieldTest.java:38:21:38:33 | problematic14 | This field is in a serializable class, but is not serializable itself because ? is not serializable. |
| NonSerializableFieldTest.java:79:10:79:20 | problematic | This field is in a serializable class, but is not serializable itself because NS is not serializable. |
| NonSerializableFieldTest.java:109:26:109:45 | nonSerializableField | This field is in a serializable class, but is not serializable itself because NonSerializableClass is not serializable. |
| NonSerializableFieldTest.java:25:8:25:19 | problematic1 | This field is in a serializable class, but is not serializable itself because NS is not serializable. |
| NonSerializableFieldTest.java:26:14:26:25 | problematic2 | This field is in a serializable class, but is not serializable itself because NS is not serializable. |
| NonSerializableFieldTest.java:27:16:27:27 | problematic3 | This field is in a serializable class, but is not serializable itself because ? is not serializable. |
| NonSerializableFieldTest.java:27:16:27:27 | problematic3 | This field is in a serializable class, but is not serializable itself because NS is not serializable. |
| NonSerializableFieldTest.java:28:16:28:27 | problematic4 | This field is in a serializable class, but is not serializable itself because ? is not serializable. |
| NonSerializableFieldTest.java:28:16:28:27 | problematic4 | This field is in a serializable class, but is not serializable itself because NS is not serializable. |
| NonSerializableFieldTest.java:29:30:29:41 | problematic5 | This field is in a serializable class, but is not serializable itself because Map<?,NS> is not serializable. |
| NonSerializableFieldTest.java:30:9:30:20 | problematic6 | This field is in a serializable class, but is not serializable itself because Map<> is not serializable. |
| NonSerializableFieldTest.java:31:24:31:35 | problematic7 | This field is in a serializable class, but is not serializable itself because ? extends NS is not serializable. |
| NonSerializableFieldTest.java:32:22:32:33 | problematic8 | This field is in a serializable class, but is not serializable itself because ? super NS is not serializable. |
| NonSerializableFieldTest.java:33:7:33:18 | problematic9 | This field is in a serializable class, but is not serializable itself because T is not serializable. |
| NonSerializableFieldTest.java:34:13:34:25 | problematic10 | This field is in a serializable class, but is not serializable itself because T is not serializable. |
| NonSerializableFieldTest.java:35:13:35:25 | problematic11 | This field is in a serializable class, but is not serializable itself because ? is not serializable. |
| NonSerializableFieldTest.java:36:15:36:27 | problematic12 | This field is in a serializable class, but is not serializable itself because ? is not serializable. |
| NonSerializableFieldTest.java:37:34:37:46 | problematic13 | This field is in a serializable class, but is not serializable itself because Map<?,Double> is not serializable. |
| NonSerializableFieldTest.java:38:21:38:33 | problematic14 | This field is in a serializable class, but is not serializable itself because ? is not serializable. |
| NonSerializableFieldTest.java:79:10:79:20 | problematic | This field is in a serializable class, but is not serializable itself because NS is not serializable. |
| NonSerializableFieldTest.java:109:26:109:45 | nonSerializableField | This field is in a serializable class, but is not serializable itself because NonSerializableClass is not serializable. |

BIN
ql/Cargo.lock generated
View File

Binary file not shown.

2
ql/extractor/Cargo.toml
View File

@@ -10,7 +10,7 @@ edition = "2018"
flate2 = "1.0"
node-types = { path = "../node-types" }
tree-sitter = "0.19"
tree-sitter-ql = { git = "https://github.com/tausbn/tree-sitter-ql.git", rev = "c3d626a77cf5acc5d8c11aaf91c12348880c8eca" }
tree-sitter-ql = { git = "https://github.com/erik-krogh/tree-sitter-ql.git", rev = "343cc5873e20510586ade803659ef8ce153bd603" }
clap = "2.33"
tracing = "0.1"
tracing-subscriber = { version = "0.3.3", features = ["env-filter"] }

2
ql/generator/Cargo.toml
View File

@@ -11,4 +11,4 @@ clap = "2.33"
node-types = { path = "../node-types" }
tracing = "0.1"
tracing-subscriber = { version = "0.3.3", features = ["env-filter"] }
tree-sitter-ql = { git = "https://github.com/tausbn/tree-sitter-ql.git", rev = "c3d626a77cf5acc5d8c11aaf91c12348880c8eca" }
tree-sitter-ql = { git = "https://github.com/erik-krogh/tree-sitter-ql.git", rev = "343cc5873e20510586ade803659ef8ce153bd603" }

92
ql/ql/src/codeql_ql/ast/Ast.qll
View File

@@ -162,6 +162,8 @@ class QLDoc extends TQLDoc, AstNode {
string getContents() { result = qldoc.getValue() }
override string getAPrimaryQlClass() { result = "QLDoc" }
override AstNode getParent() { result.getQLDoc() = this }
}
class BlockComment extends TBlockComment, AstNode {
@@ -638,7 +640,7 @@ class FieldDecl extends TFieldDecl, AstNode {
/**
* A type reference, such as `DataFlow::Node`.
*/
class TypeExpr extends TType, AstNode {
class TypeExpr extends TType, TypeRef {
QL::TypeExpr type;
TypeExpr() { this = TType(type) }
@@ -675,10 +677,14 @@ class TypeExpr extends TType, AstNode {
*/
ModuleExpr getModule() { toQL(result) = type.getQualifier() }
/**
* Gets the type that this type reference refers to.
*/
Type getResolvedType() { resolveTypeExpr(this, result) }
/** Gets the type that this type reference refers to. */
override Type getResolvedType() {
// resolve type
resolveTypeExpr(this, result)
or
// if it resolves to a module
exists(FileOrModule mod | resolveModuleRef(this, mod) | result = mod.toType())
}
override AstNode getAChild(string pred) {
result = super.getAChild(pred)
@@ -710,6 +716,9 @@ class Module extends TModule, ModuleDeclaration {
exists(int i | result = this.getMember(i) and m = this.getMember(i + 1))
}
/** Gets a ref to the module that this module implements. */
TypeExpr getImplements(int i) { toQL(result) = mod.getImplements(i).getTypeExpr() }
/** Gets the module expression that this module is an alias for, if any. */
ModuleExpr getAlias() { toQL(result) = mod.getAFieldOrChild().(QL::ModuleAliasBody).getChild() }
@@ -719,6 +728,19 @@ class Module extends TModule, ModuleDeclaration {
pred = directMember("getAlias") and result = this.getAlias()
or
pred = directMember("getAMember") and result = this.getAMember()
or
exists(int i | pred = indexedMember("getImplements", i) and result = this.getImplements(i))
or
exists(int i | pred = indexedMember("hasParameter", i) and this.hasParameter(i, _, result))
}
/** Holds if the `i`th parameter of this module has `name` and type `sig`. */
predicate hasParameter(int i, string name, SignatureExpr sig) {
exists(QL::ModuleParam param |
param = mod.getParameter(i) and
name = param.getParameter().getValue() and
sig.toQL() = param.getSignature()
)
}
}
@@ -1093,16 +1115,18 @@ class InlineCast extends TInlineCast, Expr {
}
}
/** An entity that resolves to a module. */
class ModuleRef extends AstNode, TModuleRef {
/** Gets the module that this entity resolves to. */
FileOrModule getResolvedModule() { none() }
/** An entity that resolves to a type. */
class TypeRef extends AstNode, TTypeRef {
abstract Type getResolvedType();
/** Gets the module that this entity resolves to, if this reference resolves to a module */
final FileOrModule getResolvedModule() { result.toType() = this.getResolvedType() }
}
/**
* An import statement.
*/
class Import extends TImport, ModuleMember, ModuleRef {
class Import extends TImport, ModuleMember, TypeRef {
QL::ImportDirective imp;
Import() { this = TImport(imp) }
@@ -1153,7 +1177,9 @@ class Import extends TImport, ModuleMember, ModuleRef {
)
}
final override FileOrModule getResolvedModule() { resolve(this, result) }
override Type getResolvedType() {
exists(FileOrModule mod | resolve(this, mod) | result = mod.toType())
}
}
/** A formula, such as `x = 6 and y < 5`. */
@@ -2172,7 +2198,7 @@ class DontCare extends TDontCare, Expr {
}
/** A module expression. Such as `DataFlow` in `DataFlow::Node` */
class ModuleExpr extends TModuleExpr, ModuleRef {
class ModuleExpr extends TModuleExpr, TypeRef {
QL::ModuleExpr me;
ModuleExpr() { this = TModuleExpr(me) }
@@ -2190,6 +2216,10 @@ class ModuleExpr extends TModuleExpr, ModuleRef {
result = me.getName().(QL::SimpleId).getValue()
or
not exists(me.getName()) and result = me.getChild().(QL::SimpleId).getValue()
or
exists(QL::ModuleInstantiation instantiation | instantiation.getParent() = me |
result = instantiation.getName().getChild().getValue()
)
}
/**
@@ -2203,7 +2233,9 @@ class ModuleExpr extends TModuleExpr, ModuleRef {
*/
ModuleExpr getQualifier() { result = TModuleExpr(me.getChild()) }
final override FileOrModule getResolvedModule() { resolveModuleExpr(this, result) }
override Type getResolvedType() {
exists(FileOrModule mod | resolveModuleRef(this, mod) | result = mod.toType())
}
final override string toString() { result = this.getName() }
@@ -2213,7 +2245,39 @@ class ModuleExpr extends TModuleExpr, ModuleRef {
result = super.getAChild(pred)
or
pred = directMember("getQualifier") and result = this.getQualifier()
or
exists(int i | pred = indexedMember("getArgument", i) and result = this.getArgument(i))
}
/**
* Gets the `i`th type argument if this module is a module instantiation.
* The result is either a `PredicateExpr` or a `TypeExpr`.
*/
SignatureExpr getArgument(int i) {
exists(QL::ModuleInstantiation instantiation | instantiation.getParent() = me |
result.toQL() = instantiation.getChild(i)
)
}
}
/** A signature expression, either a `PredicateExpr` or a `TypeExpr`. */
class SignatureExpr extends TSignatureExpr, AstNode {
QL::SignatureExpr sig;
SignatureExpr() {
toQL(this) = sig.getPredicate()
or
toQL(this) = sig.getTypeExpr()
}
/** Gets the generated AST node that contains this signature expression. */
QL::SignatureExpr toQL() { result = sig }
/** Gets this signature expression if it represents a predicate expression. */
PredicateExpr asPredicate() { result = this }
/** Gets this signature expression if it represents a type expression. */
TypeExpr asType() { result = this }
}
/** An argument to an annotation. */
@@ -2272,7 +2336,7 @@ class Annotation extends TAnnotation, AstNode {
/** Gets the node corresponding to the field `name`. */
string getName() { result = annot.getName().getValue() }
override AstNode getParent() { result = AstNode.super.getParent() }
override AstNode getParent() { result.getAnAnnotation() = this }
override AstNode getAChild(string pred) {
result = super.getAChild(pred)

6
ql/ql/src/codeql_ql/ast/internal/AstNodes.qll
View File

@@ -83,10 +83,12 @@ class TExpr =
class TCall = TPredicateCall or TMemberCall or TNoneCall or TAnyCall;
class TModuleRef = TImport or TModuleExpr;
class TTypeRef = TImport or TModuleExpr or TType;
class TYamlNode = TYamlCommemt or TYamlEntry or TYamlKey or TYamlListitem or TYamlValue;
class TSignatureExpr = TPredicateExpr or TType;
/** DEPRECATED: Alias for TYamlNode */
deprecated class TYAMLNode = TYamlNode;
@@ -229,4 +231,6 @@ module AstConsistency {
not node instanceof YAML::YAMLNode and // parents for YAML doens't work
not (node instanceof QLDoc and node.getLocation().getFile().getExtension() = "dbscheme") // qldoc in dbschemes are not hooked up
}
query predicate nonUniqueParent(AstNode node) { count(node.getParent()) >= 2 }
}

80
ql/ql/src/codeql_ql/ast/internal/Module.qll
View File

@@ -23,24 +23,30 @@ private class ContainerOrModule extends TContainerOrModule {
or
this = TFolder(_) and result = "folder"
}
/** Gets the module for this imported module. */
Module asModule() { this = TModule(result) }
/** Gets the file for this file. */
File asFile() { this = TFile(result) }
}
private class TFileOrModule = TFile or TModule;
/** A file or a module. */
class FileOrModule extends TFileOrModule, ContainerOrModule {
/** Gets the module for this imported module. */
Module asModule() { this = TModule(result) }
/** Gets the file for this file. */
File asFile() { this = TFile(result) }
/** Gets the file that contains this module/file. */
File getFile() {
result = this.asFile()
or
result = this.asModule().getLocation().getFile()
}
Type toType() {
result.(FileType).getDeclaration().getLocation().getFile() = this.asFile()
or
result.(ModuleType).getDeclaration() = this.asModule()
}
}
private class File_ extends FileOrModule, TFile {
@@ -207,23 +213,23 @@ private module Cached {
/** Holds if module expression `me` resolves to `m`. */
cached
predicate resolveModuleExpr(ModuleExpr me, FileOrModule m) {
predicate resolveModuleRef(TypeRef me, FileOrModule m) {
not m = TFile(any(File f | f.getExtension() = "ql")) and
not exists(me.getQualifier()) and
exists(ContainerOrModule enclosing, string name | resolveModuleExprHelper(me, enclosing, name) |
not exists(me.(ModuleExpr).getQualifier()) and
exists(ContainerOrModule enclosing, string name | resolveModuleRefHelper(me, enclosing, name) |
definesModule(enclosing, name, m, _)
)
or
exists(FileOrModule mid |
resolveModuleExpr(me.getQualifier(), mid) and
definesModule(mid, me.getName(), m, true)
resolveModuleRef(me.(ModuleExpr).getQualifier(), mid) and
definesModule(mid, me.(ModuleExpr).getName(), m, true)
)
}
pragma[noinline]
private predicate resolveModuleExprHelper(ModuleExpr me, ContainerOrModule enclosing, string name) {
private predicate resolveModuleRefHelper(TypeRef me, ContainerOrModule enclosing, string name) {
enclosing = getEnclosingModule(me).getEnclosing*() and
name = me.getName()
name = [me.(ModuleExpr).getName(), me.(TypeExpr).getClassName()]
}
}
@@ -231,7 +237,10 @@ import Cached
private import NewType
boolean getPublicBool(AstNode n) {
if n.(ModuleMember).isPrivate() or n.(NewTypeBranch).getNewType().isPrivate()
if
n.(ModuleMember).isPrivate() or
n.(NewTypeBranch).getNewType().isPrivate() or
n.(Module).isPrivate()
then result = false
else result = true
}
@@ -253,6 +262,22 @@ private predicate definesModule(
m = TModule(any(Module mod | public = getPublicBool(mod)))
)
or
// signature module in a paramertized module
exists(Module mod, SignatureExpr sig, TypeExpr ty, int i |
mod = container.asModule() and
mod.hasParameter(i, name, sig) and
public = false and
ty = sig.asType()
|
// resolve to the signature module
m = ty.getResolvedModule()
or
// resolve to the arguments of the instantiated module
exists(ModuleExpr inst | inst.getResolvedModule().asModule() = mod |
m = inst.getArgument(i).asType().getResolvedModule()
)
)
or
// import X
exists(Import imp, ContainerOrModule m0 |
container = getEnclosingModule(imp) and
@@ -274,7 +299,7 @@ private predicate definesModule(
exists(Module alias |
container = getEnclosingModule(alias) and
name = alias.getName() and
resolveModuleExpr(alias.getAlias(), m) and
resolveModuleRef(alias.getAlias(), m) and
public = getPublicBool(alias)
)
}
@@ -288,14 +313,6 @@ module ModConsistency {
.regexpMatch(".*/(test|examples|ql-training|recorded-call-graph-metrics)/.*")
}
query predicate noResolveModuleExpr(ModuleExpr me) {
not resolveModuleExpr(me, _) and
not me.getLocation()
.getFile()
.getAbsolutePath()
.regexpMatch(".*/(test|examples|ql-training|recorded-call-graph-metrics)/.*")
}
query predicate multipleResolve(Import imp, int c, ContainerOrModule m) {
c = strictcount(ContainerOrModule m0 | resolve(imp, m0)) and
c > 1 and
@@ -306,9 +323,16 @@ module ModConsistency {
.regexpMatch(".*/(test|examples|ql-training|recorded-call-graph-metrics)/.*")
}
query predicate multipleResolveModuleExpr(ModuleExpr me, int c, ContainerOrModule m) {
c = strictcount(ContainerOrModule m0 | resolveModuleExpr(me, m0)) and
c > 1 and
resolveModuleExpr(me, m)
}
// This can happen with parameterized modules.
/*
* query predicate multipleResolveModuleRef(ModuleExpr me, int c, ContainerOrModule m) {
* c = strictcount(ContainerOrModule m0 | resolveModuleRef(me, m0)) and
* c > 1 and
* resolveModuleRef(me, m)
* }
*/
query predicate noName(Module mod) { not exists(mod.getName()) }
query predicate nonUniqueName(Module mod) { count(mod.getName()) >= 2 }
}

41
ql/ql/src/codeql_ql/ast/internal/Predicate.qll
View File

@@ -69,6 +69,20 @@ private module Cached {
|
definesPredicate(m, pc.getPredicateName(), pc.getNumberOfArguments(), p, public)
)
or
exists(Module mod, PredicateExpr sig, int i |
mod.hasParameter(i, pc.getPredicateName(), sig) and
sig.getArity() = pc.getNumberOfArguments()
|
// resolve to the signature predicate
p = sig.getResolvedPredicate() // <- this is a `signature predicate`, but that's fine.
or
// resolve to the instantiations
exists(ModuleExpr inst, SignatureExpr arg | inst.getResolvedModule().asModule() = mod |
arg = inst.getArgument(i) and
p = arg.asPredicate().getResolvedPredicate()
)
)
}
private predicate resolveMemberCall(MemberCall mc, PredicateOrBuiltin p) {
@@ -162,17 +176,20 @@ module PredConsistency {
c > 1 and
resolvePredicateExpr(pe, p)
}
// This can happen with parametarized modules
/*
* query predicate multipleResolveCall(Call call, int c, PredicateOrBuiltin p) {
* c =
* strictcount(PredicateOrBuiltin p0 |
* resolveCall(call, p0) and
* // aliases are expected to resolve to multiple.
* not exists(p0.(ClasslessPredicate).getAlias()) and
* // overridden predicates may have multiple targets
* not p0.(ClassPredicate).isOverride()
* ) and
* c > 1 and
* resolveCall(call, p)
* }
*/
query predicate multipleResolveCall(Call call, int c, PredicateOrBuiltin p) {
c =
strictcount(PredicateOrBuiltin p0 |
resolveCall(call, p0) and
// aliases are expected to resolve to multiple.
not exists(p0.(ClasslessPredicate).getAlias()) and
// overridden predicates may have multiple targets
not p0.(ClassPredicate).isOverride()
) and
c > 1 and
resolveCall(call, p)
}
}

7
ql/ql/src/codeql_ql/ast/internal/TreeSitter.qll
View File

@@ -999,11 +999,16 @@ module QL {
/** Gets the name of the primary QL class for this element. */
final override string getAPrimaryQlClass() { result = "ModuleInstantiation" }
/** Gets the node corresponding to the field `name`. */
final ModuleName getName() { ql_module_instantiation_def(this, result) }
/** Gets the `i`th child of this node. */
final SignatureExpr getChild(int i) { ql_module_instantiation_child(this, i, result) }
/** Gets a field or child node of this node. */
final override AstNode getAFieldOrChild() { ql_module_instantiation_child(this, _, result) }
final override AstNode getAFieldOrChild() {
ql_module_instantiation_def(this, result) or ql_module_instantiation_child(this, _, result)
}
}
/** A class representing `moduleMember` nodes. */

58
ql/ql/src/codeql_ql/ast/internal/Type.qll
View File

@@ -14,7 +14,9 @@ private newtype TType =
TDontCare() or
TClassChar(Class c) { isActualClass(c) } or
TClassDomain(Class c) { isActualClass(c) } or
TDatabase(string s) { exists(TypeExpr t | t.isDBType() and s = t.getClassName()) }
TDatabase(string s) { exists(TypeExpr t | t.isDBType() and s = t.getClassName()) } or
TFile(TopLevel t) or
TModule(Module m)
private predicate primTypeName(string s) { s = ["int", "float", "string", "boolean", "date"] }
@@ -110,6 +112,26 @@ class ClassType extends Type, TClass {
}
}
class FileType extends Type, TFile {
TopLevel decl;
FileType() { this = TFile(decl) }
override string getName() { result = decl.getLocation().getFile().getBaseName() }
override TopLevel getDeclaration() { result = decl }
}
class ModuleType extends Type, TModule {
Module decl;
ModuleType() { this = TModule(decl) }
override string getName() { result = decl.getName() }
override Module getDeclaration() { result = decl }
}
private PredicateOrBuiltin declaredPred(Type ty, string name, int arity) {
result.getDeclaringType() = ty and
result.getName() = name and
@@ -323,22 +345,42 @@ private predicate defines(FileOrModule m, string name, Type t, boolean public) {
public = getPublicBool(im) and
defines(im.getResolvedModule(), name, t, true)
)
or
// classes in parameterized modules.
exists(Module mod, SignatureExpr param, int i |
m.asModule() = mod and
mod.hasParameter(i, name, param) and
public = true
|
// resolve to the signature class
t = param.asType().getResolvedType()
or
// resolve to the instantiations
exists(ModuleExpr inst, SignatureExpr arg |
inst.getArgument(i) = arg and
inst.getResolvedModule() = m and
t = arg.asType().getResolvedType()
)
)
}
module TyConsistency {
query predicate noResolve(TypeExpr te) {
not resolveTypeExpr(te, _) and
query predicate noResolve(TypeRef te) {
not exists(te.getResolvedType()) and
not te.getLocation()
.getFile()
.getAbsolutePath()
.regexpMatch(".*/(test|examples|ql-training|recorded-call-graph-metrics)/.*")
}
query predicate multipleResolve(TypeExpr te, int c, Type t) {
c = strictcount(Type t0 | resolveTypeExpr(te, t0)) and
c > 1 and
resolveTypeExpr(te, t)
}
// This can happen with parameterized modules.
/*
* query predicate multipleResolve(TypeExpr te, int c, Type t) {
* c = strictcount(Type t0 | resolveTypeExpr(te, t0)) and
* c > 1 and
* resolveTypeExpr(te, t)
* }
*/
query predicate multiplePrimitives(TypeExpr te, int c, PrimitiveType t) {
c = strictcount(PrimitiveType t0 | resolveTypeExpr(te, t0)) and

6
ql/ql/src/codeql_ql/style/DeadCodeQuery.qll
View File

@@ -164,6 +164,12 @@ private AstNode aliveStep(AstNode prev) {
result = prev.(Annotation).getAChild()
or
result = prev.(Predicate).getReturnType().getDeclaration()
or
// a module parameter is alive is the module is alive
prev.(Module).hasParameter(_, _, result)
or
// the implements of a module
result = prev.(Module).getImplements(_)
}
private AstNode deprecated() {

5
ql/ql/src/ide-contextual-queries/Definitions.qll
View File

@@ -28,14 +28,15 @@ class Loc extends TLoc {
}
}
private predicate resolveModule(ModuleRef ref, FileOrModule target, string kind) {
private predicate resolveModule(TypeRef ref, FileOrModule target, string kind) {
target = ref.getResolvedModule() and
kind = "module"
}
private predicate resolveType(TypeExpr ref, AstNode target, string kind) {
target = ref.getResolvedType().getDeclaration() and
kind = "type"
kind = "type" and
not resolveModule(ref, _, _) // modules are types, so we exclude them here.
}
private predicate resolvePredicate(PredicateExpr ref, Predicate target, string kind) {

3
ql/ql/src/ql.dbscheme
View File

@@ -656,7 +656,8 @@ ql_module_instantiation_child(
);
ql_module_instantiation_def(
unique int id: @ql_module_instantiation
unique int id: @ql_module_instantiation,
int name: @ql_module_name ref
);
@ql_moduleMember_child_type = @ql_annotation | @ql_classless_predicate | @ql_dataclass | @ql_datatype | @ql_import_directive | @ql_module | @ql_select | @ql_token_qldoc

6
ql/ql/src/queries/diagnostics/EmptyConsistencies.ql
View File

@@ -33,11 +33,15 @@ where
or
AstConsistency::nonTotalGetParent(node) and msg = "AstConsistency::nonTotalGetParent"
or
AstConsistency::nonUniqueParent(node) and msg = "AstConsistency::nonUniqueParent"
or
TypeConsistency::noResolve(node) and msg = "TypeConsistency::noResolve"
or
ModConsistency::noResolve(node) and msg = "ModConsistency::noResolve"
or
ModConsistency::noResolveModuleExpr(node) and msg = "ModConsistency::noResolveModuleExpr"
ModConsistency::noName(node) and msg = "ModConsistency::noName"
or
ModConsistency::nonUniqueName(node) and msg = "ModConsistency::nonUniqueName"
or
VarConsistency::noFieldDef(node) and msg = "VarConsistency::noFieldDef"
or

63
ql/ql/test/callgraph/ParamModules.qll Normal file
View File

@@ -0,0 +1,63 @@
module PredicateSig {
signature predicate fooSig(int i);
module UsesFoo<fooSig/1 fooImpl> {
predicate bar(int i) { fooImpl(i + 1) }
}
predicate myFoo(int i) { i = 42 }
predicate use(int i) { UsesFoo<myFoo/1>::bar(i) }
}
module ClassSig {
signature class FooSig extends int;
module UsesFoo<FooSig FooImpl> {
FooImpl getAnEven() { result % 2 = 0 }
}
class MyFoo extends int {
MyFoo() { this = [0 .. 10] }
string myFoo() { result = "myFoo" }
}
string use() { result = UsesFoo<MyFoo>::getAnEven().myFoo() }
}
module ModuleSig {
signature module FooSig {
class A;
A getThing();
}
module UsesFoo<FooSig FooImpl> {
B getThing() { result = FooImpl::getThing() }
class B = FooImpl::A;
}
module MyFoo implements FooSig {
class C extends int {
C() { this = [0 .. 10] }
string myFoo() { result = "myFoo" }
}
class A = C;
C getThing() { any() }
}
module ImplStuff {
module Inst = UsesFoo<MyFoo>;
class D = Inst::B;
string use1() { result = Inst::getThing().myFoo() }
string use2(Inst::B b) { result = b.myFoo() }
}
}

12
ql/ql/test/callgraph/callgraph.expected
View File

@@ -21,6 +21,16 @@ getTarget
| Overrides.qll:24:39:24:48 | MemberCall | Overrides.qll:22:12:22:44 | ClassPredicate bar |
| Overrides.qll:28:3:28:9 | MemberCall | Overrides.qll:6:3:6:29 | ClassPredicate bar |
| Overrides.qll:29:3:29:10 | MemberCall | Overrides.qll:8:3:8:41 | ClassPredicate baz |
| ParamModules.qll:5:28:5:41 | PredicateCall | ParamModules.qll:2:13:2:36 | ClasslessPredicate fooSig |
| ParamModules.qll:5:28:5:41 | PredicateCall | ParamModules.qll:8:3:8:35 | ClasslessPredicate myFoo |
| ParamModules.qll:10:26:10:49 | PredicateCall | ParamModules.qll:5:5:5:43 | ClasslessPredicate bar |
| ParamModules.qll:26:27:26:53 | PredicateCall | ParamModules.qll:17:5:17:42 | ClasslessPredicate getAnEven |
| ParamModules.qll:26:27:26:61 | MemberCall | ParamModules.qll:23:5:23:39 | ClassPredicate myFoo |
| ParamModules.qll:37:29:37:47 | PredicateCall | ParamModules.qll:33:5:33:17 | ClasslessPredicate getThing |
| ParamModules.qll:37:29:37:47 | PredicateCall | ParamModules.qll:51:5:51:26 | ClasslessPredicate getThing |
| ParamModules.qll:59:30:59:45 | PredicateCall | ParamModules.qll:37:5:37:49 | ClasslessPredicate getThing |
| ParamModules.qll:59:30:59:53 | MemberCall | ParamModules.qll:46:7:46:41 | ClassPredicate myFoo |
| ParamModules.qll:61:39:61:47 | MemberCall | ParamModules.qll:46:7:46:41 | ClassPredicate myFoo |
| packs/other/OtherThing.qll:5:3:5:8 | PredicateCall | packs/lib/LibThing/Foo.qll:1:1:1:30 | ClasslessPredicate foo |
| packs/other/OtherThing.qll:6:3:6:8 | PredicateCall | packs/src/SrcThing.qll:8:1:8:30 | ClasslessPredicate bar |
| packs/src/SrcThing.qll:4:3:4:8 | PredicateCall | packs/lib/LibThing/Foo.qll:1:1:1:30 | ClasslessPredicate foo |
@@ -33,3 +43,5 @@ exprPredicate
| Foo.qll:26:22:26:31 | predicate | Foo.qll:20:3:20:54 | ClasslessPredicate myThing2 |
| Foo.qll:47:55:47:62 | predicate | Foo.qll:42:20:42:27 | NewTypeBranch MkRoot |
| Foo.qll:47:65:47:70 | predicate | Foo.qll:44:9:44:56 | ClasslessPredicate edge |
| ParamModules.qll:4:18:4:25 | predicate | ParamModules.qll:2:13:2:36 | ClasslessPredicate fooSig |
| ParamModules.qll:10:34:10:40 | predicate | ParamModules.qll:8:3:8:35 | ClasslessPredicate myFoo |

BIN
ruby/Cargo.lock generated
View File

Binary file not shown.

9
ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll
View File

@@ -240,7 +240,7 @@ abstract class ActiveRecordModelInstantiation extends OrmInstantiation::Range,
// Names of class methods on ActiveRecord models that may return one or more
// instances of that model. This also includes the `initialize` method.
// See https://api.rubyonrails.org/classes/ActiveRecord/FinderMethods.html
private string finderMethodName() {
private string staticFinderMethodName() {
exists(string baseName |
baseName =
[
@@ -287,7 +287,12 @@ private class ActiveRecordModelFinderCall extends ActiveRecordModelInstantiation
callScope = cls.getAMethod()
)
) and
call.getMethodName() = finderMethodName()
(
call.getMethodName() = staticFinderMethodName()
or
// dynamically generated finder methods
call.getMethodName().indexOf("find_by_") = 0
)
)
}

87
ruby/ql/lib/codeql/ruby/security/ImproperMemoizationQuery.qll Normal file
View File

@@ -0,0 +1,87 @@
/**
* Provides predicates for reasoning about improper memoization methods.
*/
private import ruby
private import codeql.ruby.DataFlow
private import codeql.ruby.dataflow.internal.DataFlowDispatch
/**
* A `||=` statement that memoizes a value by storing it in an instance variable.
*/
private class MemoStmt extends AssignLogicalOrExpr {
private InstanceVariableAccess instanceVariable;
MemoStmt() { instanceVariable.getParent*() = this.getLeftOperand() }
/**
* Gets the variable access on the LHS of this statement.
* This is the `@a` in `@a ||= e`.
*/
VariableAccess getVariableAccess() { result = instanceVariable }
}
/**
* A `||=` statement that stores a value at a particular location in a Hash,
* which itself is stored in an instance variable.
* For example:
* ```rb
* @a[key] ||= e
* ```
*/
private class HashMemoStmt extends MemoStmt {
HashMemoStmt() {
exists(ElementReference e |
e = this.getLeftOperand() and this.getVariableAccess().getParent+() = e
)
}
}
/**
* A method that may be performing memoization.
*/
private class MemoCandidate extends Method {
MemoCandidate() { this = any(MemoStmt m).getEnclosingMethod() }
}
/**
* Holds if parameter `p` of `m` is read in the right hand side of `assign`.
*/
private predicate parameterUsedInMemoValue(Method m, Parameter p, MemoStmt a) {
p = m.getAParameter() and
a.getEnclosingMethod() = m and
p.getAVariable().getAnAccess().getParent*() = a.getRightOperand()
}
/**
* Holds if parameter `p` of `m` is read in the left hand side of `assign`.
*/
private predicate parameterUsedInMemoKey(Method m, Parameter p, HashMemoStmt a) {
p = m.getAParameter() and
a.getEnclosingMethod() = m and
p.getAVariable().getAnAccess().getParent+() = a.getLeftOperand()
}
/**
* Holds if the assignment `s` is returned from its parent method `m`.
*/
private predicate memoReturnedFromMethod(Method m, MemoStmt s) {
exists(DataFlow::Node n | n.asExpr().getExpr() = s and exprNodeReturnedFrom(n, m))
or
// If we don't have flow (e.g. due to the dataflow library not supporting instance variable flow yet),
// fall back to a syntactic heuristic: does the last statement in the method mention the memoization variable?
m.getLastStmt().getAChild*().(InstanceVariableReadAccess).getVariable() =
s.getVariableAccess().getVariable()
}
/**
* Holds if `m` is a memoization method with a parameter `p` which is not used in the memoization key.
* This can cause stale or incorrect values to be returned when the method is called with different arguments.
*/
predicate isImproperMemoizationMethod(Method m, Parameter p, AssignLogicalOrExpr s) {
m instanceof MemoCandidate and
m.getName() != "initialize" and
parameterUsedInMemoValue(m, p, s) and
not parameterUsedInMemoKey(m, p, s) and
memoReturnedFromMethod(m, s)
}

4
ruby/ql/src/change-notes/2022-05-24-improper-memoization.md Normal file
View File

@@ -0,0 +1,4 @@
---
category: newQuery
---
* Added a new query, `rb/improper-memoization`. The query finds cases where the parameter of a memoization method is not used in the memoization key.

84
ruby/ql/src/experimental/improper-memoization/ImproperMemoization.qhelp Normal file
View File

@@ -0,0 +1,84 @@
<!DOCTYPE qhelp PUBLIC
"-//Semmle//qhelp//EN"
"qhelp.dtd">
<qhelp>
<overview>
<p>
A common pattern in Ruby is to memoize the result of a method by storing
it in an instance variable. If the method has no parameters, it can simply
be stored directly in a variable.
</p>
<sample language="ruby">
def answer
@answer ||= calculate_answer
end
</sample>
<p>
If the method takes parameters, then there are multiple results to store
(one for each combination of parameter value). In this case the values
should be stored in a hash, keyed by the parameter values.
</p>
<sample language="ruby">
def answer(x, y)
@answer ||= {}
@answer[x] ||= {}
@answer[x][y] ||= calculate_answer
end
</sample>
<p>
If a memoization method takes parameters but does not include them in the
memoization key, subsequent calls to the method with different parameter
values may incorrectly return the same result. This can lead to the method
returning stale data, or leaking sensitive information.
</p>
</overview>
<example>
<p>
In this example, the method does not include its parameters in the
memoization key. The first call to this method will cache the result, and
subsequent calls will return the same result regardless of what arguments
are given.
</p>
<sample language="ruby">
def answer(x, y)
@answer ||= calculate_answer(x, y)
end
</sample>
<p>
This can be fixed by storing the result of <code>calculate_answer</code>
in a hash, keyed by the parameter values.
</p>
<sample language="ruby">
def answer(x, y)
@answer ||= {}
@answer[x] ||= {}
@answer[x][y] ||= calculate_answer(x, y)
end
</sample>
<p>
Note that if the result of <code>calculate_answer</code> is
<code>false</code> or <code>nil</code>, then it will not be cached.
To cache these values you can use a different pattern:
</p>
<sample language="ruby">
def answer(x, y)
@answer ||= Hash.new do |h1, x|
h1[x] = Hash.new do |h2, y|
h2[y] = calculate_answer(x, y)
end
end
@answer[x][y]
end
</sample>
</example>
</qhelp>

17
ruby/ql/src/experimental/improper-memoization/ImproperMemoization.ql Normal file
View File

@@ -0,0 +1,17 @@
/**
* @name Improper Memoization
* @description Omitting a parameter from the key of a memoization method can lead to reading stale or incorrect data.
* @kind problem
* @problem.severity warning
* @precision high
* @tags security
* @id rb/improper-memoization
*/
import ruby
import codeql.ruby.security.ImproperMemoizationQuery
from Method m, Parameter p, AssignLogicalOrExpr s
where isImproperMemoizationMethod(m, p, s)
select m, "A $@ in this memoization method does not form part of the memoization key.", p,
"parameter"

66
ruby/ql/test/library-tests/frameworks/ActionController.expected
View File

@@ -1,17 +1,17 @@
actionControllerControllerClasses
| ActiveRecordInjection.rb:27:1:58:3 | FooController |
| ActiveRecordInjection.rb:60:1:90:3 | BarController |
| ActiveRecordInjection.rb:92:1:96:3 | BazController |
| ActiveRecord.rb:23:1:39:3 | FooController |
| ActiveRecord.rb:41:1:64:3 | BarController |
| ActiveRecord.rb:66:1:70:3 | BazController |
| app/controllers/comments_controller.rb:1:1:7:3 | CommentsController |
| app/controllers/foo/bars_controller.rb:3:1:31:3 | BarsController |
| app/controllers/photos_controller.rb:1:1:4:3 | PhotosController |
| app/controllers/posts_controller.rb:1:1:10:3 | PostsController |
| app/controllers/users/notifications_controller.rb:2:3:5:5 | NotificationsController |
actionControllerActionMethods
| ActiveRecordInjection.rb:32:3:57:5 | some_request_handler |
| ActiveRecordInjection.rb:61:3:69:5 | some_other_request_handler |
| ActiveRecordInjection.rb:71:3:89:5 | safe_paths |
| ActiveRecordInjection.rb:93:3:95:5 | yet_another_handler |
| ActiveRecord.rb:27:3:38:5 | some_request_handler |
| ActiveRecord.rb:42:3:47:5 | some_other_request_handler |
| ActiveRecord.rb:49:3:63:5 | safe_paths |
| ActiveRecord.rb:67:3:69:5 | yet_another_handler |
| app/controllers/comments_controller.rb:2:3:3:5 | index |
| app/controllers/comments_controller.rb:5:3:6:5 | show |
| app/controllers/foo/bars_controller.rb:5:3:7:5 | index |
@@ -23,38 +23,38 @@ actionControllerActionMethods
| app/controllers/posts_controller.rb:8:3:9:5 | upvote |
| app/controllers/users/notifications_controller.rb:3:5:4:7 | mark_as_read |
paramsCalls
| ActiveRecordInjection.rb:35:30:35:35 | call to params |
| ActiveRecordInjection.rb:39:29:39:34 | call to params |
| ActiveRecordInjection.rb:43:31:43:36 | call to params |
| ActiveRecordInjection.rb:48:21:48:26 | call to params |
| ActiveRecordInjection.rb:54:34:54:39 | call to params |
| ActiveRecordInjection.rb:56:23:56:28 | call to params |
| ActiveRecordInjection.rb:56:38:56:43 | call to params |
| ActiveRecordInjection.rb:62:10:62:15 | call to params |
| ActiveRecordInjection.rb:72:11:72:16 | call to params |
| ActiveRecordInjection.rb:77:12:77:17 | call to params |
| ActiveRecordInjection.rb:83:12:83:17 | call to params |
| ActiveRecordInjection.rb:88:15:88:20 | call to params |
| ActiveRecordInjection.rb:94:21:94:26 | call to params |
| ActiveRecord.rb:28:30:28:35 | call to params |
| ActiveRecord.rb:29:29:29:34 | call to params |
| ActiveRecord.rb:30:31:30:36 | call to params |
| ActiveRecord.rb:32:21:32:26 | call to params |
| ActiveRecord.rb:34:34:34:39 | call to params |
| ActiveRecord.rb:35:23:35:28 | call to params |
| ActiveRecord.rb:35:38:35:43 | call to params |
| ActiveRecord.rb:43:10:43:15 | call to params |
| ActiveRecord.rb:50:11:50:16 | call to params |
| ActiveRecord.rb:54:12:54:17 | call to params |
| ActiveRecord.rb:59:12:59:17 | call to params |
| ActiveRecord.rb:62:15:62:20 | call to params |
| ActiveRecord.rb:68:21:68:26 | call to params |
| app/controllers/foo/bars_controller.rb:13:21:13:26 | call to params |
| app/controllers/foo/bars_controller.rb:14:10:14:15 | call to params |
| app/controllers/foo/bars_controller.rb:21:21:21:26 | call to params |
| app/controllers/foo/bars_controller.rb:22:10:22:15 | call to params |
| app/views/foo/bars/show.html.erb:5:9:5:14 | call to params |
paramsSources
| ActiveRecordInjection.rb:35:30:35:35 | call to params |
| ActiveRecordInjection.rb:39:29:39:34 | call to params |
| ActiveRecordInjection.rb:43:31:43:36 | call to params |
| ActiveRecordInjection.rb:48:21:48:26 | call to params |
| ActiveRecordInjection.rb:54:34:54:39 | call to params |
| ActiveRecordInjection.rb:56:23:56:28 | call to params |
| ActiveRecordInjection.rb:56:38:56:43 | call to params |
| ActiveRecordInjection.rb:62:10:62:15 | call to params |
| ActiveRecordInjection.rb:72:11:72:16 | call to params |
| ActiveRecordInjection.rb:77:12:77:17 | call to params |
| ActiveRecordInjection.rb:83:12:83:17 | call to params |
| ActiveRecordInjection.rb:88:15:88:20 | call to params |
| ActiveRecordInjection.rb:94:21:94:26 | call to params |
| ActiveRecord.rb:28:30:28:35 | call to params |
| ActiveRecord.rb:29:29:29:34 | call to params |
| ActiveRecord.rb:30:31:30:36 | call to params |
| ActiveRecord.rb:32:21:32:26 | call to params |
| ActiveRecord.rb:34:34:34:39 | call to params |
| ActiveRecord.rb:35:23:35:28 | call to params |
| ActiveRecord.rb:35:38:35:43 | call to params |
| ActiveRecord.rb:43:10:43:15 | call to params |
| ActiveRecord.rb:50:11:50:16 | call to params |
| ActiveRecord.rb:54:12:54:17 | call to params |
| ActiveRecord.rb:59:12:59:17 | call to params |
| ActiveRecord.rb:62:15:62:20 | call to params |
| ActiveRecord.rb:68:21:68:26 | call to params |
| app/controllers/foo/bars_controller.rb:13:21:13:26 | call to params |
| app/controllers/foo/bars_controller.rb:14:10:14:15 | call to params |
| app/controllers/foo/bars_controller.rb:21:21:21:26 | call to params |

112
ruby/ql/test/library-tests/frameworks/ActiveRecord.expected
View File

@@ -1,60 +1,64 @@
activeRecordModelClasses
| ActiveRecordInjection.rb:1:1:3:3 | UserGroup |
| ActiveRecordInjection.rb:5:1:17:3 | User |
| ActiveRecordInjection.rb:19:1:25:3 | Admin |
| ActiveRecord.rb:1:1:3:3 | UserGroup |
| ActiveRecord.rb:5:1:15:3 | User |
| ActiveRecord.rb:17:1:21:3 | Admin |
activeRecordInstances
| ActiveRecordInjection.rb:10:5:10:68 | call to find |
| ActiveRecordInjection.rb:15:5:15:40 | call to find_by |
| ActiveRecordInjection.rb:79:5:81:7 | if ... |
| ActiveRecordInjection.rb:79:43:80:40 | then ... |
| ActiveRecordInjection.rb:80:7:80:40 | call to find_by |
| ActiveRecordInjection.rb:85:5:85:33 | call to find_by |
| ActiveRecordInjection.rb:88:5:88:34 | call to find |
| ActiveRecord.rb:9:5:9:68 | call to find |
| ActiveRecord.rb:13:5:13:40 | call to find_by |
| ActiveRecord.rb:36:5:36:30 | call to find_by_name |
| ActiveRecord.rb:55:5:57:7 | if ... |
| ActiveRecord.rb:55:43:56:40 | then ... |
| ActiveRecord.rb:56:7:56:40 | call to find_by |
| ActiveRecord.rb:60:5:60:33 | call to find_by |
| ActiveRecord.rb:62:5:62:34 | call to find |
activeRecordSqlExecutionRanges
| ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" |
| ActiveRecordInjection.rb:23:16:23:24 | condition |
| ActiveRecordInjection.rb:35:30:35:44 | ...[...] |
| ActiveRecordInjection.rb:39:20:39:42 | "id = '#{...}'" |
| ActiveRecordInjection.rb:43:22:43:44 | "id = '#{...}'" |
| ActiveRecordInjection.rb:47:16:47:21 | <<-SQL |
| ActiveRecordInjection.rb:54:20:54:47 | "user.id = '#{...}'" |
| ActiveRecordInjection.rb:68:20:68:32 | ... + ... |
| ActiveRecordInjection.rb:75:16:75:28 | "name #{...}" |
| ActiveRecordInjection.rb:80:20:80:39 | "username = #{...}" |
| ActiveRecord.rb:9:33:9:67 | "name='#{...}' and pass='#{...}'" |
| ActiveRecord.rb:19:16:19:24 | condition |
| ActiveRecord.rb:28:30:28:44 | ...[...] |
| ActiveRecord.rb:29:20:29:42 | "id = '#{...}'" |
| ActiveRecord.rb:30:22:30:44 | "id = '#{...}'" |
| ActiveRecord.rb:31:16:31:21 | <<-SQL |
| ActiveRecord.rb:34:20:34:47 | "user.id = '#{...}'" |
| ActiveRecord.rb:46:20:46:32 | ... + ... |
| ActiveRecord.rb:52:16:52:28 | "name #{...}" |
| ActiveRecord.rb:56:20:56:39 | "username = #{...}" |
activeRecordModelClassMethodCalls
| ActiveRecordInjection.rb:2:3:2:17 | call to has_many |
| ActiveRecordInjection.rb:6:3:6:24 | call to belongs_to |
| ActiveRecordInjection.rb:10:5:10:68 | call to find |
| ActiveRecordInjection.rb:15:5:15:40 | call to find_by |
| ActiveRecordInjection.rb:15:5:15:46 | call to users |
| ActiveRecordInjection.rb:23:5:23:25 | call to destroy_by |
| ActiveRecordInjection.rb:35:5:35:45 | call to calculate |
| ActiveRecordInjection.rb:39:5:39:43 | call to delete_by |
| ActiveRecordInjection.rb:43:5:43:46 | call to destroy_by |
| ActiveRecordInjection.rb:47:5:47:35 | call to where |
| ActiveRecordInjection.rb:54:5:54:14 | call to where |
| ActiveRecordInjection.rb:54:5:54:48 | call to not |
| ActiveRecordInjection.rb:56:5:56:51 | call to authenticate |
| ActiveRecordInjection.rb:68:5:68:33 | call to delete_by |
| ActiveRecordInjection.rb:75:5:75:29 | call to order |
| ActiveRecordInjection.rb:80:7:80:40 | call to find_by |
| ActiveRecordInjection.rb:85:5:85:33 | call to find_by |
| ActiveRecordInjection.rb:88:5:88:34 | call to find |
| ActiveRecordInjection.rb:94:5:94:45 | call to delete_by |
| ActiveRecord.rb:2:3:2:17 | call to has_many |
| ActiveRecord.rb:6:3:6:24 | call to belongs_to |
| ActiveRecord.rb:9:5:9:68 | call to find |
| ActiveRecord.rb:13:5:13:40 | call to find_by |
| ActiveRecord.rb:13:5:13:46 | call to users |
| ActiveRecord.rb:19:5:19:25 | call to destroy_by |
| ActiveRecord.rb:28:5:28:45 | call to calculate |
| ActiveRecord.rb:29:5:29:43 | call to delete_by |
| ActiveRecord.rb:30:5:30:46 | call to destroy_by |
| ActiveRecord.rb:31:5:31:35 | call to where |
| ActiveRecord.rb:34:5:34:14 | call to where |
| ActiveRecord.rb:34:5:34:48 | call to not |
| ActiveRecord.rb:35:5:35:51 | call to authenticate |
| ActiveRecord.rb:36:5:36:30 | call to find_by_name |
| ActiveRecord.rb:37:5:37:36 | call to not_a_find_by_method |
| ActiveRecord.rb:46:5:46:33 | call to delete_by |
| ActiveRecord.rb:52:5:52:29 | call to order |
| ActiveRecord.rb:56:7:56:40 | call to find_by |
| ActiveRecord.rb:60:5:60:33 | call to find_by |
| ActiveRecord.rb:62:5:62:34 | call to find |
| ActiveRecord.rb:68:5:68:45 | call to delete_by |
potentiallyUnsafeSqlExecutingMethodCall
| ActiveRecordInjection.rb:10:5:10:68 | call to find |
| ActiveRecordInjection.rb:23:5:23:25 | call to destroy_by |
| ActiveRecordInjection.rb:35:5:35:45 | call to calculate |
| ActiveRecordInjection.rb:39:5:39:43 | call to delete_by |
| ActiveRecordInjection.rb:43:5:43:46 | call to destroy_by |
| ActiveRecordInjection.rb:47:5:47:35 | call to where |
| ActiveRecordInjection.rb:54:5:54:48 | call to not |
| ActiveRecordInjection.rb:68:5:68:33 | call to delete_by |
| ActiveRecordInjection.rb:75:5:75:29 | call to order |
| ActiveRecordInjection.rb:80:7:80:40 | call to find_by |
| ActiveRecord.rb:9:5:9:68 | call to find |
| ActiveRecord.rb:19:5:19:25 | call to destroy_by |
| ActiveRecord.rb:28:5:28:45 | call to calculate |
| ActiveRecord.rb:29:5:29:43 | call to delete_by |
| ActiveRecord.rb:30:5:30:46 | call to destroy_by |
| ActiveRecord.rb:31:5:31:35 | call to where |
| ActiveRecord.rb:34:5:34:48 | call to not |
| ActiveRecord.rb:46:5:46:33 | call to delete_by |
| ActiveRecord.rb:52:5:52:29 | call to order |
| ActiveRecord.rb:56:7:56:40 | call to find_by |
activeRecordModelInstantiations
| ActiveRecordInjection.rb:10:5:10:68 | call to find | ActiveRecordInjection.rb:5:1:17:3 | User |
| ActiveRecordInjection.rb:15:5:15:40 | call to find_by | ActiveRecordInjection.rb:1:1:3:3 | UserGroup |
| ActiveRecordInjection.rb:80:7:80:40 | call to find_by | ActiveRecordInjection.rb:5:1:17:3 | User |
| ActiveRecordInjection.rb:85:5:85:33 | call to find_by | ActiveRecordInjection.rb:5:1:17:3 | User |
| ActiveRecordInjection.rb:88:5:88:34 | call to find | ActiveRecordInjection.rb:5:1:17:3 | User |
| ActiveRecord.rb:9:5:9:68 | call to find | ActiveRecord.rb:5:1:15:3 | User |
| ActiveRecord.rb:13:5:13:40 | call to find_by | ActiveRecord.rb:1:1:3:3 | UserGroup |
| ActiveRecord.rb:36:5:36:30 | call to find_by_name | ActiveRecord.rb:5:1:15:3 | User |
| ActiveRecord.rb:56:7:56:40 | call to find_by | ActiveRecord.rb:5:1:15:3 | User |
| ActiveRecord.rb:60:5:60:33 | call to find_by | ActiveRecord.rb:5:1:15:3 | User |
| ActiveRecord.rb:62:5:62:34 | call to find | ActiveRecord.rb:5:1:15:3 | User |

32
ruby/ql/test/library-tests/frameworks/ActiveRecordInjection.rb → ruby/ql/test/library-tests/frameworks/ActiveRecord.rb
View File

@@ -6,20 +6,16 @@ class User < ApplicationRecord
belongs_to :user_group
def self.authenticate(name, pass)
# BAD: possible untrusted input interpolated into SQL fragment
find(:first, :conditions => "name='#{name}' and pass='#{pass}'")
end
def self.from(user_group_id)
# GOOD: `find_by` with hash argument
UserGroup.find_by(id: user_group_id).users
end
end
class Admin < User
def self.delete_by(condition = nil)
# BAD: `delete_by` overrides an ActiveRecord method, but doesn't perform
# any validation before passing its arguments on to another ActiveRecord method
destroy_by(condition)
end
end
@@ -28,32 +24,17 @@ class FooController < ActionController::Base
MAX_USER_ID = 100_000
# A string tainted by user input is inserted into an SQL query
def some_request_handler
# BAD: executes `SELECT AVG(#{params[:column]}) FROM "users"`
# where `params[:column]` is unsanitized
User.calculate(:average, params[:column])
# BAD: executes `DELETE FROM "users" WHERE (id = '#{params[:id]}')`
# where `params[:id]` is unsanitized
User.delete_by("id = '#{params[:id]}'")
# BAD: executes `SELECT "users".* FROM "users" WHERE (id = '#{params[:id]}')`
# where `params[:id]` is unsanitized
User.destroy_by(["id = '#{params[:id]}'"])
# BAD: executes `SELECT "users".* FROM "users" WHERE id BETWEEN '#{params[:min_id]}' AND 100000`
# where `params[:min_id]` is unsanitized
User.where(<<-SQL, MAX_USER_ID)
id BETWEEN '#{params[:min_id]}' AND ?
SQL
# BAD: chained method case
# executes `SELECT "users".* FROM "users" WHERE (NOT (user_id = 'params[:id]'))`
# where `params[:id]` is unsanitized
User.where.not("user.id = '#{params[:id]}'")
User.authenticate(params[:name], params[:pass])
User.find_by_name("alice")
User.not_a_find_by_method("bob")
end
end
@@ -62,29 +43,22 @@ class BarController < ApplicationController
ps = params
uid = ps[:id]
uidEq = "= '#{uid}'"
# BAD: executes `DELETE FROM "users" WHERE (id = #{uid})`
# where `uid` is unsantized
User.delete_by("id " + uidEq)
end
def safe_paths
dir = params[:order]
# GOOD: barrier guard prevents taint flow
dir = "DESC" unless dir == "ASC"
User.order("name #{dir}")
name = params[:user_name]
# GOOD: barrier guard prevents taint flow
if %w(alice bob charlie).include? name
User.find_by("username = #{name}")
end
name = params[:user_name]
# GOOD: hash arguments are sanitized by ActiveRecord
User.find_by(user_name: name)
# OK: `find` method is overridden in `User`
User.find(params[:user_group])
end
end
@@ -93,4 +67,4 @@ class BazController < BarController
def yet_another_handler
Admin.delete_by(params[:admin_condition])
end
end
end

12
ruby/ql/test/query-tests/experimental/improper-memoization/ImproperMemoization.expected Normal file
View File

@@ -0,0 +1,12 @@
failures
| improper_memoization.rb:100:1:104:3 | m14 | Unexpected result: result=BAD |
#select
| improper_memoization.rb:50:1:55:3 | m7 | improper_memoization.rb:50:8:50:10 | arg | improper_memoization.rb:51:3:53:5 | ... \|\|= ... |
| improper_memoization.rb:58:1:63:3 | m8 | improper_memoization.rb:58:8:58:10 | arg | improper_memoization.rb:59:3:61:5 | ... \|\|= ... |
| improper_memoization.rb:66:1:68:3 | m9 | improper_memoization.rb:66:8:66:10 | arg | improper_memoization.rb:67:3:67:34 | ... \|\|= ... |
| improper_memoization.rb:71:1:73:3 | m10 | improper_memoization.rb:71:9:71:12 | arg1 | improper_memoization.rb:72:3:72:42 | ... \|\|= ... |
| improper_memoization.rb:71:1:73:3 | m10 | improper_memoization.rb:71:15:71:18 | arg2 | improper_memoization.rb:72:3:72:42 | ... \|\|= ... |
| improper_memoization.rb:76:1:79:3 | m11 | improper_memoization.rb:76:15:76:18 | arg2 | improper_memoization.rb:78:3:78:48 | ... \|\|= ... |
| improper_memoization.rb:82:1:87:3 | m12 | improper_memoization.rb:82:15:82:18 | arg2 | improper_memoization.rb:83:3:85:5 | ... \|\|= ... |
| improper_memoization.rb:90:1:97:3 | m13 | improper_memoization.rb:90:9:90:10 | id | improper_memoization.rb:91:3:95:5 | ... \|\|= ... |
| improper_memoization.rb:100:1:104:3 | m14 | improper_memoization.rb:100:9:100:11 | arg | improper_memoization.rb:103:3:103:40 | ... \|\|= ... |

23
ruby/ql/test/query-tests/experimental/improper-memoization/ImproperMemoization.ql Normal file
View File

@@ -0,0 +1,23 @@
import ruby
import TestUtilities.InlineExpectationsTest
import codeql.ruby.security.ImproperMemoizationQuery
class ImproperMemoizationTest extends InlineExpectationsTest {
ImproperMemoizationTest() { this = "ImproperMemoizationTest" }
override string getARelevantTag() { result = "BAD" }
override predicate hasActualResult(Location location, string element, string tag, string value) {
tag = "result" and
value = "BAD" and
exists(Expr e |
isImproperMemoizationMethod(e, _, _) and
location = e.getLocation() and
element = e.toString()
)
}
}
from Method m, Parameter p, AssignLogicalOrExpr s
where isImproperMemoizationMethod(m, p, s)
select m, p, s

104
ruby/ql/test/query-tests/experimental/improper-memoization/improper_memoization.rb Normal file
View File

@@ -0,0 +1,104 @@
# GOOD - Should not trigger CodeQL rule
# No arguments passed to method
def m1
@m1 ||= long_running_method
end
# No arguments passed to method
def m2
@m2 ||= begin
long_running_method
end
end
# OK: argument used in key.
# May be incorrect if arg is `false` or `nil`.
def m3(arg)
@m3 ||= {}
@m3[arg] ||= long_running_method(arg)
end
# OK: both arguments used in key.
# May be incorrect if either arg is `false` or `nil`.
def m4(arg1, arg2)
@m4 ||= {}
@m4[[arg1, arg2]] ||= result(arg1, arg2)
end
# OK: argument used in key.
# Still correct if arg is `false` or `nil`.
def m5(arg)
@m5 ||= Hash.new do |h1, key|
h1[key] = long_running_method(key)
end
@m5[arg]
end
# OK: both arguments used in key.
# Still correct if either arg is `false` or `nil`.
def m6(arg1, arg2)
@m6 ||= Hash.new do |h1, arg1|
h1[arg1] = Hash.new do |h2, arg2|
h2[arg2] = result(arg1, arg2)
end
end
@m6[arg1][arg2]
end
# Bad: method has parameter but only one result is memoized.
def m7(arg) # $result=BAD
@m7 ||= begin
arg += 3
end
@m7
end
# Bad: method has parameter but only one result is memoized.
def m8(arg) # $result=BAD
@m8 ||= begin
long_running_method(arg)
end
@m8
end
# Bad: method has parameter but only one result is memoized.
def m9(arg) # $result=BAD
@m9 ||= long_running_method(arg)
end
# Bad: method has parameter but only one result is memoized.
def m10(arg1, arg2) # $result=BAD
@m10 ||= long_running_method(arg1, arg2)
end
# Bad: `arg2` not used in key.
def m11(arg1, arg2) # $result=BAD
@m11 ||= {}
@m11[arg1] ||= long_running_method(arg1, arg2)
end
# Bad: `arg2` not used in key.
def m12(arg1, arg2) # $result=BAD
@m12 ||= Hash.new do |h1, arg1|
h1[arg1] = result(arg1, arg2)
end
@m12[arg1]
end
# Bad: arg not used in key.
def m13(id:) # $result=BAD
@m13 ||= Rails.cache.fetch("product_sku/#{id}", expires_in: 30.minutes) do
ActiveRecord::Base.transaction do
ProductSku.find_by(id: id)
end
end
@m13
end
# Good (FP): arg is used in key via string interpolation.
def m14(arg)
@m14 ||= {}
key = "foo/#{arg}"
@m14[key] ||= long_running_method(arg)
end