Description
Note: Fully recognize that this may wind up being an unacceptable ask based on the amount of inspection or Facts needed to be surfaced by the static analyzer.
Within our code base we have some multi level maps (i.e. map[key1]map[key2]value). For these data types, we have code that is used to abstract the act of initializing the second layer if absent. This is essentially computeIfAbsent behaviour (if the key is absent, compute and set the value for that field in the map)
I found that even though nilaway could identify within the scope of a method that the map field was initialized, abstracting the computeIfAbsent behaviour to a method causes a false positive. While looking into this, I also found that mixing usage of constants ("key") and variables (k := "key") within the scope of a method causes a false positive. A toy example showing all of the points can be seen below:
Sample Code
package main
func setByMethod_Flagged() {
outMap := map[string]*int{}
_computeIfAbsent(outMap, "a")
*(outMap["a"]) = 1 // flagged, doesn't panic (FP)
}
func mixedSet_Flagged() {
outMap := map[string]*int{}
key := "a"
value, present := outMap[key]
if !present {
value = new(int)
outMap[key] = value
}
*(outMap["a"]) = 1 // flagged, doesn't panic (FP)
}
func _computeIfAbsent(mapping map[string]*int, key string) {
value, present := mapping[key]
if !present {
value = new(int)
mapping[key] = value
}
}
func setWithVariable_NotFlagged() {
outMap := map[string]*int{}
key := "a"
value, present := outMap[key]
if !present {
value = new(int)
outMap[key] = value
}
*(outMap[key]) = 1 // not flagged, doesn't panic (TN)
}Nilaway:
~/niltest/main.go:6:3: error: Potential nil panic detected. Observed nil flow from source to dereference point:
- niltest/main.go:6:3: deep read from local variable `outMap` lacking guarding; dereferenced
~/niltest/main.go:19:3: error: Potential nil panic detected. Observed nil flow from source to dereference point:
- niltest/main.go:19:3: deep read from local variable `outMap` lacking guarding; dereferenced
Extended examples:
Here are some extra cases that show correct behaviour
func setWithVariable_NotFlagged() {
outMap := map[string]*int{}
key := "a"
value, present := outMap[key]
if !present {
value = new(int)
outMap[key] = value
}
*(outMap[key]) = 1 // not flagged, doesn't panic (TN)
}
func notSetByMethod_Flagged() {
outMap := map[string]*int{}
_computeIfAbsent(outMap, "a")
*(outMap["b"]) = 1 // flagged, panics (TP)
}
func notSet_Flagged() {
outMap := map[string]*int{}
*(outMap["b"]) = 1 // flagged, panics (TP)
}
func setWithConstant_NotFlagged() {
outMap := map[string]*int{}
value, present := outMap["a"]
if !present {
value = new(int)
outMap["a"] = value
}
*(outMap["a"]) = 1 // not flagged, doesn't panic (TN)
}Nilaway:
~/niltest/main.go:33:3: error: Potential nil panic detected. Observed nil flow from source to dereference point:
- niltest/main.go:33:3: deep read from local variable `outMap` lacking guarding; dereferenced
~/niltest/main.go:38:3: error: Potential nil panic detected. Observed nil flow from source to dereference point:
- niltest/main.go:38:3: deep read from local variable `outMap` lacking guarding; dereferenced