From 5475b317515305d60ee81dc6fc897a366d3686da Mon Sep 17 00:00:00 2001 From: Zac Blazic Date: Tue, 17 Mar 2020 21:32:43 +0200 Subject: [PATCH 1/4] Prevent index out of range error when passenger process is killed If any passenger process that is position `n-1` in the list of processes is removed (usually due to it being killed), the very next call to `updateProcesses` will panic with an index out of range error. The two factors at play here are: * The length of the `found` array. * The index values of the remaning processes. If we have 3 processes indexed as follows: * 0 - `p1` * 1 - `p2` * 2 - `p3` Assume the first process is killed, and we then call `updateProcesses`, we get the following output: * 1 - `p2` * 2 - `p3` Time has passed and we call `updateProcesses` once again. When we do so, the `old` process list is as follows: * 1 - `p2` * 2 - `p3` Note that the `old` process list has a length of 2 but contains an entry with an index that is larger than the length of the map. When the `found` array is accessed with the `p3` index, we get the index out of range error. TLDR; If you remove `p1` or `p2`, you still have `p3` with an index of 2, yet the length of the `found` array is 2. --- passenger_exporter.go | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/passenger_exporter.go b/passenger_exporter.go index 322f63d..96f629e 100644 --- a/passenger_exporter.go +++ b/passenger_exporter.go @@ -276,7 +276,7 @@ func (e *Exporter) Collect(ch chan<- prometheus.Metric) { ch <- prometheus.MustNewConstMetric(e.appProcsSpawning, prometheus.GaugeValue, parseFloat(sg.Group.ProcessesSpawning), sg.Name) // Update process identifiers map. - processIdentifiers = updateProcesses(processIdentifiers, sg.Group.Processes) + processIdentifiers = updateProcesses(processIdentifiers, sg.Group.Processes, parseInt(info.MaxProcessCount)) for _, proc := range sg.Group.Processes { if bucketID, ok := processIdentifiers[proc.PID]; ok { ch <- prometheus.MustNewConstMetric(e.procMemory, prometheus.GaugeValue, parseFloat(proc.RealMemory), sg.Name, strconv.Itoa(bucketID)) @@ -345,6 +345,15 @@ func parseFloat(val string) float64 { return v } +func parseInt(val string) int { + v, err := strconv.Atoi(val) + if err != nil { + log.Errorf("failed to parse %s: %v", val, err) + v = 0 + } + return v +} + // updateProcesses updates the global map from process id:exporter id. Process // TTLs cause new processes to be created on a user-defined cycle. When a new // process replaces an old process, the new process's statistics will be @@ -356,10 +365,10 @@ func parseFloat(val string) float64 { // process/pid appears, it is mapped to either the first empty place // within the global map storing process identifiers, or mapped to // pid:id pair in the map. -func updateProcesses(old map[string]int, processes []Process) map[string]int { +func updateProcesses(old map[string]int, processes []Process, maxProcesses int) map[string]int { var ( updated = make(map[string]int) - found = make([]string, len(old)) + found = make([]string, maxProcesses) missing []string ) From 65f7f4b32405ea2b2152ae49a4258f72756a8100 Mon Sep 17 00:00:00 2001 From: Zac Blazic Date: Tue, 17 Mar 2020 21:54:11 +0200 Subject: [PATCH 2/4] Add test cases for panic --- passenger_exporter_test.go | 52 +++++++++++++++++++++++++++++++------- 1 file changed, 43 insertions(+), 9 deletions(-) diff --git a/passenger_exporter_test.go b/passenger_exporter_test.go index edfc4de..3a6bcbb 100644 --- a/passenger_exporter_test.go +++ b/passenger_exporter_test.go @@ -115,23 +115,26 @@ func TestStatusTimeout(t *testing.T) { } type updateProcessSpec struct { - name string - input map[string]int - processes []Process - output map[string]int + name string + input map[string]int + processes []Process + maxProcesses int + output map[string]int } func newUpdateProcessSpec( name string, input map[string]int, processes []Process, + maxProcesses int, ) updateProcessSpec { s := updateProcessSpec{ - name: name, - input: input, - processes: processes, + name: name, + input: input, + processes: processes, + maxProcesses: maxProcesses, } - s.output = updateProcesses(s.input, s.processes) + s.output = updateProcesses(s.input, s.processes, s.maxProcesses) return s } @@ -145,6 +148,7 @@ func TestUpdateProcessIdentifiers(t *testing.T) { Process{PID: "cdf"}, Process{PID: "dfe"}, }, + 3, ), newUpdateProcessSpec( "1:1", @@ -158,6 +162,7 @@ func TestUpdateProcessIdentifiers(t *testing.T) { Process{PID: "cdf"}, Process{PID: "dfe"}, }, + 6, ), newUpdateProcessSpec( "increase processes", @@ -174,6 +179,7 @@ func TestUpdateProcessIdentifiers(t *testing.T) { Process{PID: "jkl"}, Process{PID: "lmn"}, }, + 9, ), newUpdateProcessSpec( "reduce processes", @@ -190,6 +196,33 @@ func TestUpdateProcessIdentifiers(t *testing.T) { Process{PID: "cdf"}, Process{PID: "dfe"}, }, + 3, + ), + newUpdateProcessSpec( + "first process killed", + map[string]int{ + "abc": 0, + "cdf": 1, + "dfe": 2, + }, + []Process{ + Process{PID: "cdf"}, + Process{PID: "dfe"}, + }, + 3, + ), + newUpdateProcessSpec( + "second process killed", + map[string]int{ + "abc": 0, + "cdf": 1, + "dfe": 2, + }, + []Process{ + Process{PID: "abc"}, + Process{PID: "dfe"}, + }, + 3, ), } { if len(spec.output) != len(spec.processes) { @@ -202,7 +235,7 @@ func TestUpdateProcessIdentifiers(t *testing.T) { } } - newOutput := updateProcesses(spec.output, spec.processes) + newOutput := updateProcesses(spec.output, spec.processes, spec.maxProcesses) if !reflect.DeepEqual(newOutput, spec.output) { t.Fatalf("case %s: updateProcesses is not idempotent", spec.name) } @@ -224,6 +257,7 @@ func TestInsertingNewProcesses(t *testing.T) { Process{PID: "newPID"}, Process{PID: "newPID2"}, }, + 6, ) if len(spec.output) != len(spec.processes) { From c8ddaae2b0a1e7c025464ad6ba4c23777bd81ed5 Mon Sep 17 00:00:00 2001 From: Zac Blazic Date: Tue, 17 Mar 2020 21:54:43 +0200 Subject: [PATCH 3/4] Bump version and update changelog --- CHANGELOG.md | 5 +++++ VERSION | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d3c3717..80836c2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,10 @@ # Changelog +## 0.7.1 + +### Bug Fixes +* Prevent index out of range panics when passenger processes are killed. + ## 0.7.0 * Change group to `nobody` instead of `nogroup`. diff --git a/VERSION b/VERSION index 4b9fcbe..39e898a 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.5.1 +0.7.1 From 67ec97b6b8cd953fe9cd54b7cf7256abeef0c08e Mon Sep 17 00:00:00 2001 From: Zac Blazic Date: Wed, 18 Mar 2020 09:51:44 +0200 Subject: [PATCH 4/4] Ensure max processes value is >= input map and processes array --- passenger_exporter_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/passenger_exporter_test.go b/passenger_exporter_test.go index 3a6bcbb..2ef0ed9 100644 --- a/passenger_exporter_test.go +++ b/passenger_exporter_test.go @@ -196,7 +196,7 @@ func TestUpdateProcessIdentifiers(t *testing.T) { Process{PID: "cdf"}, Process{PID: "dfe"}, }, - 3, + 6, ), newUpdateProcessSpec( "first process killed",