From 68fb7606d91dd40a376f84bb2a71203fa1d51a24 Mon Sep 17 00:00:00 2001 From: Jakub Sobon Date: Wed, 20 Mar 2019 23:20:16 -0400 Subject: [PATCH] Prevent external mutation of data passed into widgets. Fixes #174. --- CHANGELOG.md | 3 + widgets/barchart/barchart.go | 7 ++- widgets/barchart/barchart_test.go | 55 +++++++++++++++++++ widgets/barchart/options.go | 4 +- widgets/linechart/internal/axes/scale.go | 5 ++ widgets/linechart/linechart.go | 23 +++++--- widgets/linechart/linechart_test.go | 47 ++++++++++++++++ widgets/segmentdisplay/segmentdisplay_test.go | 34 ++++++++++++ widgets/sparkline/sparkline_test.go | 24 ++++++++ 9 files changed, 192 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 56257fc..6b3f768 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 not just received them. - Container focus tracker now correctly tracks focus changes in enlarged areas, i.e. when the terminal size increased. +- The BarChart, LineChart and SegmentDisplay widgets now protect against + external mutation of the values passed into them by copying the data they + receive. #### Breaking API changes diff --git a/widgets/barchart/barchart.go b/widgets/barchart/barchart.go index 31418cd..3ab4e85 100644 --- a/widgets/barchart/barchart.go +++ b/widgets/barchart/barchart.go @@ -264,14 +264,17 @@ func (bc *BarChart) Values(values []int, max int, opts ...Option) error { bc.mu.Lock() defer bc.mu.Unlock() - if err := validateValues(values, max); err != nil { + // Copy to avoid external modifications. See #174. + v := make([]int, len(values)) + copy(v, values) + if err := validateValues(v, max); err != nil { return err } for _, opt := range opts { opt.set(bc.opts) } - bc.values = values + bc.values = v bc.max = max return nil } diff --git a/widgets/barchart/barchart_test.go b/widgets/barchart/barchart_test.go index ffad520..eb66bf5 100644 --- a/widgets/barchart/barchart_test.go +++ b/widgets/barchart/barchart_test.go @@ -579,6 +579,61 @@ func TestBarChart(t *testing.T) { }, wantCapacity: 3, }, + { + desc: "regression for #174, protects against external data mutation", + opts: []Option{ + Char('o'), + Labels([]string{ + "1", + "2", + "3", + }), + }, + update: func(bc *BarChart) error { + values := []int{1, 2, 5, 10} + if err := bc.Values(values, 10); err != nil { + return err + } + values[0] = 100 + return nil + }, + canvas: image.Rect(0, 0, 7, 11), + want: func(size image.Point) *faketerm.Terminal { + ft := faketerm.MustNew(size) + c := testcanvas.MustNew(ft.Area()) + + testdraw.MustRectangle(c, image.Rect(0, 9, 1, 10), + draw.RectChar('o'), + draw.RectCellOpts(cell.BgColor(DefaultBarColor)), + ) + testdraw.MustRectangle(c, image.Rect(2, 8, 3, 10), + draw.RectChar('o'), + draw.RectCellOpts(cell.BgColor(DefaultBarColor)), + ) + testdraw.MustRectangle(c, image.Rect(4, 5, 5, 10), + draw.RectChar('o'), + draw.RectCellOpts(cell.BgColor(DefaultBarColor)), + ) + testdraw.MustRectangle(c, image.Rect(6, 0, 7, 10), + draw.RectChar('o'), + draw.RectCellOpts(cell.BgColor(DefaultBarColor)), + ) + + // Labels. + testdraw.MustText(c, "1", image.Point{0, 10}, draw.TextCellOpts( + cell.FgColor(DefaultLabelColor), + )) + testdraw.MustText(c, "2", image.Point{2, 10}, draw.TextCellOpts( + cell.FgColor(DefaultLabelColor), + )) + testdraw.MustText(c, "3", image.Point{4, 10}, draw.TextCellOpts( + cell.FgColor(DefaultLabelColor), + )) + testcanvas.MustApply(c, ft) + return ft + }, + wantCapacity: 4, + }, } for _, tc := range tests { diff --git a/widgets/barchart/options.go b/widgets/barchart/options.go index 1bdb770..3806132 100644 --- a/widgets/barchart/options.go +++ b/widgets/barchart/options.go @@ -143,7 +143,9 @@ func LabelColors(colors []cell.Color) Option { // label. func Labels(labels []string) Option { return option(func(opts *options) { - opts.labels = labels + // Copy to avoid external modifications. See #174. + opts.labels = make([]string, len(labels)) + copy(opts.labels, labels) }) } diff --git a/widgets/linechart/internal/axes/scale.go b/widgets/linechart/internal/axes/scale.go index 4802944..202e696 100644 --- a/widgets/linechart/internal/axes/scale.go +++ b/widgets/linechart/internal/axes/scale.go @@ -68,6 +68,11 @@ type YScale struct { brailleHeight int } +// String implements fmt.Stringer. +func (ys *YScale) String() string { + return fmt.Sprintf("YScale{Min:%v, Max:%v, Step:%v, GraphHeight:%v}", ys.Min, ys.Max, ys.Step, ys.GraphHeight) +} + // NewYScale calculates the scale of the Y axis, given the boundary values and // the height of the graph. The nonZeroDecimals dictates rounding of the // calculated scale, see NewValue for details. diff --git a/widgets/linechart/linechart.go b/widgets/linechart/linechart.go index e3e778c..d656168 100644 --- a/widgets/linechart/linechart.go +++ b/widgets/linechart/linechart.go @@ -52,9 +52,13 @@ type seriesValues struct { // newSeriesValues returns a new seriesValues instance. func newSeriesValues(values []float64) *seriesValues { - min, max := numbers.MinMax(values) + // Copy to avoid external modifications. See #174. + v := make([]float64, len(values)) + copy(v, values) + + min, max := numbers.MinMax(v) return &seriesValues{ - values: values, + values: v, min: min, max: max, } @@ -146,7 +150,12 @@ func SeriesCellOpts(co ...cell.Option) SeriesOption { func SeriesXLabels(labels map[int]string) SeriesOption { return seriesOption(func(opts *seriesValues) { opts.xLabelsSet = true - opts.xLabels = labels + + // Copy to avoid external modifications. See #174. + opts.xLabels = make(map[int]string, len(labels)) + for pos, label := range labels { + opts.xLabels[pos] = label + } }) } @@ -409,21 +418,21 @@ func (lc *LineChart) drawSeries(cvs *canvas.Canvas, xd *axes.XDetails, yd *axes. startX, err := xdZoomed.Scale.ValueToPixel(i - 1) if err != nil { - return nil, fmt.Errorf("failure for series %v[%d], xdZoomed.Scale.ValueToPixel => %v", name, i-1, err) + return nil, fmt.Errorf("failure for series %v[%d] on scale %v, xdZoomed.Scale.ValueToPixel(%v) => %v", name, i-1, xdZoomed.Scale, i-1, err) } endX, err := xdZoomed.Scale.ValueToPixel(i) if err != nil { - return nil, fmt.Errorf("failure for series %v[%d], xdZoomed.Scale.ValueToPixel => %v", name, i, err) + return nil, fmt.Errorf("failure for series %v[%d] on scale %v, xdZoomed.Scale.ValueToPixel(%v) => %v", name, i, xdZoomed.Scale, i, err) } startY, err := yd.Scale.ValueToPixel(prev) if err != nil { - return nil, fmt.Errorf("failure for series %v[%d], yd.Scale.ValueToPixel => %v", name, i-1, err) + return nil, fmt.Errorf("failure for series %v[%d] on scale %v, yd.Scale.ValueToPixel(%v) => %v", name, i-1, yd.Scale, prev, err) } v := sv.values[i] endY, err := yd.Scale.ValueToPixel(v) if err != nil { - return nil, fmt.Errorf("failure for series %v[%d], yd.Scale.ValueToPixel => %v", name, i, err) + return nil, fmt.Errorf("failure for series %v[%d] on scale %v, yd.Scale.ValueToPixel(%v) => %v", name, i, yd.Scale, v, err) } if err := draw.BrailleLine(bc, diff --git a/widgets/linechart/linechart_test.go b/widgets/linechart/linechart_test.go index 325f21d..9c5fbf8 100644 --- a/widgets/linechart/linechart_test.go +++ b/widgets/linechart/linechart_test.go @@ -1479,6 +1479,53 @@ func TestLineChartDraws(t *testing.T) { return ft }, }, + { + desc: "regression for #174, protects against external data mutation", + canvas: image.Rect(0, 0, 20, 10), + writes: func(lc *LineChart) error { + values := []float64{0, 100} + labels := map[int]string{ + 0: "start", + 1: "end", + } + if err := lc.Series("first", values, SeriesXLabels(labels)); err != nil { + return err + } + + // Modify the values after they were passed in. + // Increase above the previous maximum to run out of the Y axis. + // The linechart should not use the original values. + values[1] = 1000 + labels[0] = "bad" + return nil + }, + wantCapacity: 28, + want: func(size image.Point) *faketerm.Terminal { + ft := faketerm.MustNew(size) + c := testcanvas.MustNew(ft.Area()) + + // Y and X axis. + lines := []draw.HVLine{ + {Start: image.Point{5, 0}, End: image.Point{5, 8}}, + {Start: image.Point{5, 8}, End: image.Point{19, 8}}, + } + testdraw.MustHVLines(c, lines) + + // Value labels. + testdraw.MustText(c, "0", image.Point{4, 7}) + testdraw.MustText(c, "51.68", image.Point{0, 3}) + testdraw.MustText(c, "start", image.Point{6, 9}) + + // Braille line. + graphAr := image.Rect(6, 0, 20, 8) + bc := testbraille.MustNew(graphAr) + testdraw.MustBrailleLine(bc, image.Point{0, 31}, image.Point{26, 0}) + testbraille.MustCopyTo(bc, c) + + testcanvas.MustApply(c, ft) + return ft + }, + }, } for _, tc := range tests { diff --git a/widgets/segmentdisplay/segmentdisplay_test.go b/widgets/segmentdisplay/segmentdisplay_test.go index 38b9c41..eb6fff6 100644 --- a/widgets/segmentdisplay/segmentdisplay_test.go +++ b/widgets/segmentdisplay/segmentdisplay_test.go @@ -731,6 +731,40 @@ func TestSegmentDisplay(t *testing.T) { mustDrawChar(cvs, tc.char, tc.area) } + testcanvas.MustApply(cvs, ft) + return ft + }, + }, + { + desc: "regression for #174, protects against external data mutation", + opts: []Option{ + GapPercent(0), + }, + canvas: image.Rect(0, 0, sixteen.MinCols*3, sixteen.MinRows), + update: func(sd *SegmentDisplay) error { + chunks := []*TextChunk{NewChunk("123")} + if err := sd.Write(chunks); err != nil { + return err + } + // Mutates and adds unsupported characters. + chunks[0] = NewChunk("\r\t") + return nil + }, + want: func(size image.Point) *faketerm.Terminal { + ft := faketerm.MustNew(size) + cvs := testcanvas.MustNew(ft.Area()) + + for _, tc := range []struct { + char rune + area image.Rectangle + }{ + {'1', image.Rect(0, 0, sixteen.MinCols, sixteen.MinRows)}, + {'2', image.Rect(sixteen.MinCols, 0, sixteen.MinCols*2, sixteen.MinRows)}, + {'3', image.Rect(sixteen.MinCols*2, 0, sixteen.MinCols*3, sixteen.MinRows)}, + } { + mustDrawChar(cvs, tc.char, tc.area) + } + testcanvas.MustApply(cvs, ft) return ft }, diff --git a/widgets/sparkline/sparkline_test.go b/widgets/sparkline/sparkline_test.go index b7c9c31..9681628 100644 --- a/widgets/sparkline/sparkline_test.go +++ b/widgets/sparkline/sparkline_test.go @@ -426,6 +426,30 @@ func TestSparkLine(t *testing.T) { }, wantCapacity: 2, }, + { + desc: "regression for #174, protects against external data mutation", + update: func(sl *SparkLine) error { + values := []int{0, 1, 2, 3, 4, 5, 6, 7, 8} + if err := sl.Add(values); err != nil { + return err + } + // Mutation should have no effect. + values[0] = 8 + return nil + }, + canvas: image.Rect(0, 0, 9, 1), + want: func(size image.Point) *faketerm.Terminal { + ft := faketerm.MustNew(size) + c := testcanvas.MustNew(ft.Area()) + + testdraw.MustText(c, "▁▂▃▄▅▆▇█", image.Point{1, 0}, draw.TextCellOpts( + cell.FgColor(DefaultColor), + )) + testcanvas.MustApply(c, ft) + return ft + }, + wantCapacity: 9, + }, } for _, tc := range tests {