From 9c4b0334f21eab6156445ca874db7e2f0de980b1 Mon Sep 17 00:00:00 2001 From: Aliaksandr Valialkin Date: Wed, 17 Jul 2024 13:52:10 +0200 Subject: [PATCH] all: consistently use stringsutil.JSONString() for formatting JSON strings with fmt.* functions instead of using "%q" formatter The %q formatter may result in incorrectly formatted JSON string if the original string contains special chars such as \x1b . They must be encoded as \u001b , otherwise the resulting JSON string cannot be parsed by JSON parsers. This is a follow-up for c0caa6993903a748c8942c8c28b5901ee6d5f4d4 See https://github.com/VictoriaMetrics/victorialogs-datasource/issues/24 --- app/vlogsgenerator/main.go | 2 +- app/vmagent/main.go | 6 ++++-- app/vminsert/main.go | 3 ++- app/vmselect/promql/active_queries.go | 6 ++++-- app/vmselect/querystats/querystats.go | 11 +++++----- app/vmstorage/main.go | 8 +++++--- lib/httpserver/httpserver.go | 10 ++++++---- lib/promscrape/config_test.go | 4 ++-- lib/promscrape/targetstatus.go | 13 ++++++------ .../opentelemetry/firehose/http.go | 4 +++- lib/streamaggr/streamaggr_timing_test.go | 4 ++-- lib/stringsutil/json.go | 10 ++++++++++ lib/stringsutil/json_test.go | 20 +++++++++++++++++++ 13 files changed, 72 insertions(+), 29 deletions(-) create mode 100644 lib/stringsutil/json.go create mode 100644 lib/stringsutil/json_test.go diff --git a/app/vlogsgenerator/main.go b/app/vlogsgenerator/main.go index 516201a78..c91ba7be7 100644 --- a/app/vlogsgenerator/main.go +++ b/app/vlogsgenerator/main.go @@ -230,7 +230,7 @@ func generateLogsAtTimestamp(bw *bufio.Writer, workerID int, ts int64, firstStre for i := 0; i < activeStreams; i++ { ip := toIPv4(rand.Uint32()) uuid := toUUID(rand.Uint64(), rand.Uint64()) - fmt.Fprintf(bw, `{"_time":%q,"_msg":"message for the stream %d and worker %d; ip=%s; uuid=%s; u64=%d","host":"host_%d","worker_id":"%d"`, + fmt.Fprintf(bw, `{"_time":"%s","_msg":"message for the stream %d and worker %d; ip=%s; uuid=%s; u64=%d","host":"host_%d","worker_id":"%d"`, timeStr, streamID, workerID, ip, uuid, rand.Uint64(), streamID, workerID) fmt.Fprintf(bw, `,"run_id":"%s"`, runID) for j := 0; j < *constFieldsPerLog; j++ { diff --git a/app/vmagent/main.go b/app/vmagent/main.go index 26c77dbf3..302fbc831 100644 --- a/app/vmagent/main.go +++ b/app/vmagent/main.go @@ -10,6 +10,8 @@ import ( "strings" "time" + "github.com/VictoriaMetrics/metrics" + "github.com/VictoriaMetrics/VictoriaMetrics/app/vmagent/csvimport" "github.com/VictoriaMetrics/VictoriaMetrics/app/vmagent/datadogsketches" "github.com/VictoriaMetrics/VictoriaMetrics/app/vmagent/datadogv1" @@ -42,7 +44,7 @@ import ( "github.com/VictoriaMetrics/VictoriaMetrics/lib/protoparser/common" "github.com/VictoriaMetrics/VictoriaMetrics/lib/protoparser/opentelemetry/firehose" "github.com/VictoriaMetrics/VictoriaMetrics/lib/pushmetrics" - "github.com/VictoriaMetrics/metrics" + "github.com/VictoriaMetrics/VictoriaMetrics/lib/stringsutil" ) var ( @@ -450,7 +452,7 @@ func requestHandler(w http.ResponseWriter, r *http.Request) bool { w.Header().Set("Content-Type", "application/json") var bb bytesutil.ByteBuffer promscrape.WriteConfigData(&bb) - fmt.Fprintf(w, `{"status":"success","data":{"yaml":%q}}`, bb.B) + fmt.Fprintf(w, `{"status":"success","data":{"yaml":%s}}`, stringsutil.JSONString(string(bb.B))) return true case "/prometheus/-/reload", "/-/reload": if !httpserver.CheckAuthFlag(w, r, reloadAuthKey) { diff --git a/app/vminsert/main.go b/app/vminsert/main.go index e5c639654..f3bed07be 100644 --- a/app/vminsert/main.go +++ b/app/vminsert/main.go @@ -42,6 +42,7 @@ import ( "github.com/VictoriaMetrics/VictoriaMetrics/lib/protoparser/common" "github.com/VictoriaMetrics/VictoriaMetrics/lib/protoparser/opentelemetry/firehose" "github.com/VictoriaMetrics/VictoriaMetrics/lib/storage" + "github.com/VictoriaMetrics/VictoriaMetrics/lib/stringsutil" ) var ( @@ -343,7 +344,7 @@ func RequestHandler(w http.ResponseWriter, r *http.Request) bool { w.Header().Set("Content-Type", "application/json") var bb bytesutil.ByteBuffer promscrape.WriteConfigData(&bb) - fmt.Fprintf(w, `{"status":"success","data":{"yaml":%q}}`, bb.B) + fmt.Fprintf(w, `{"status":"success","data":{"yaml":%s}}`, stringsutil.JSONString(string(bb.B))) return true case "/prometheus/-/reload", "/-/reload": if !httpserver.CheckAuthFlag(w, r, reloadAuthKey) { diff --git a/app/vmselect/promql/active_queries.go b/app/vmselect/promql/active_queries.go index ccad4170f..776d63146 100644 --- a/app/vmselect/promql/active_queries.go +++ b/app/vmselect/promql/active_queries.go @@ -7,6 +7,8 @@ import ( "sync" "sync/atomic" "time" + + "github.com/VictoriaMetrics/VictoriaMetrics/lib/stringsutil" ) // ActiveQueriesHandler returns response to /api/v1/status/active_queries @@ -23,8 +25,8 @@ func ActiveQueriesHandler(w http.ResponseWriter, _ *http.Request) { fmt.Fprintf(w, `{"status":"ok","data":[`) for i, aqe := range aqes { d := now.Sub(aqe.startTime) - fmt.Fprintf(w, `{"duration":"%.3fs","id":"%016X","remote_addr":%s,"query":%q,"start":%d,"end":%d,"step":%d}`, - d.Seconds(), aqe.qid, aqe.quotedRemoteAddr, aqe.q, aqe.start, aqe.end, aqe.step) + fmt.Fprintf(w, `{"duration":"%.3fs","id":"%016X","remote_addr":%s,"query":%s,"start":%d,"end":%d,"step":%d}`, + d.Seconds(), aqe.qid, aqe.quotedRemoteAddr, stringsutil.JSONString(aqe.q), aqe.start, aqe.end, aqe.step) if i+1 < len(aqes) { fmt.Fprintf(w, `,`) } diff --git a/app/vmselect/querystats/querystats.go b/app/vmselect/querystats/querystats.go index 26ce3834c..59ac05699 100644 --- a/app/vmselect/querystats/querystats.go +++ b/app/vmselect/querystats/querystats.go @@ -9,6 +9,7 @@ import ( "time" "github.com/VictoriaMetrics/VictoriaMetrics/lib/logger" + "github.com/VictoriaMetrics/VictoriaMetrics/lib/stringsutil" ) var ( @@ -74,13 +75,13 @@ func initQueryStats() { } func (qst *queryStatsTracker) writeJSONQueryStats(w io.Writer, topN int, maxLifetime time.Duration) { - fmt.Fprintf(w, `{"topN":"%d","maxLifetime":%q,`, topN, maxLifetime) + fmt.Fprintf(w, `{"topN":"%d","maxLifetime":"%s",`, topN, maxLifetime) fmt.Fprintf(w, `"search.queryStats.lastQueriesCount":%d,`, *lastQueriesCount) - fmt.Fprintf(w, `"search.queryStats.minQueryDuration":%q,`, *minQueryDuration) + fmt.Fprintf(w, `"search.queryStats.minQueryDuration":"%s",`, *minQueryDuration) fmt.Fprintf(w, `"topByCount":[`) topByCount := qst.getTopByCount(topN, maxLifetime) for i, r := range topByCount { - fmt.Fprintf(w, `{"query":%q,"timeRangeSeconds":%d,"count":%d}`, r.query, r.timeRangeSecs, r.count) + fmt.Fprintf(w, `{"query":%s,"timeRangeSeconds":%d,"count":%d}`, stringsutil.JSONString(r.query), r.timeRangeSecs, r.count) if i+1 < len(topByCount) { fmt.Fprintf(w, `,`) } @@ -88,7 +89,7 @@ func (qst *queryStatsTracker) writeJSONQueryStats(w io.Writer, topN int, maxLife fmt.Fprintf(w, `],"topByAvgDuration":[`) topByAvgDuration := qst.getTopByAvgDuration(topN, maxLifetime) for i, r := range topByAvgDuration { - fmt.Fprintf(w, `{"query":%q,"timeRangeSeconds":%d,"avgDurationSeconds":%.3f,"count":%d}`, r.query, r.timeRangeSecs, r.duration.Seconds(), r.count) + fmt.Fprintf(w, `{"query":%s,"timeRangeSeconds":%d,"avgDurationSeconds":%.3f,"count":%d}`, stringsutil.JSONString(r.query), r.timeRangeSecs, r.duration.Seconds(), r.count) if i+1 < len(topByAvgDuration) { fmt.Fprintf(w, `,`) } @@ -96,7 +97,7 @@ func (qst *queryStatsTracker) writeJSONQueryStats(w io.Writer, topN int, maxLife fmt.Fprintf(w, `],"topBySumDuration":[`) topBySumDuration := qst.getTopBySumDuration(topN, maxLifetime) for i, r := range topBySumDuration { - fmt.Fprintf(w, `{"query":%q,"timeRangeSeconds":%d,"sumDurationSeconds":%.3f,"count":%d}`, r.query, r.timeRangeSecs, r.duration.Seconds(), r.count) + fmt.Fprintf(w, `{"query":%s,"timeRangeSeconds":%d,"sumDurationSeconds":%.3f,"count":%d}`, stringsutil.JSONString(r.query), r.timeRangeSecs, r.duration.Seconds(), r.count) if i+1 < len(topBySumDuration) { fmt.Fprintf(w, `,`) } diff --git a/app/vmstorage/main.go b/app/vmstorage/main.go index b7fe762e2..d496310f9 100644 --- a/app/vmstorage/main.go +++ b/app/vmstorage/main.go @@ -21,6 +21,7 @@ import ( "github.com/VictoriaMetrics/VictoriaMetrics/lib/mergeset" "github.com/VictoriaMetrics/VictoriaMetrics/lib/querytracer" "github.com/VictoriaMetrics/VictoriaMetrics/lib/storage" + "github.com/VictoriaMetrics/VictoriaMetrics/lib/stringsutil" "github.com/VictoriaMetrics/VictoriaMetrics/lib/syncwg" "github.com/VictoriaMetrics/VictoriaMetrics/lib/timeutil" ) @@ -308,9 +309,9 @@ func RequestHandler(w http.ResponseWriter, r *http.Request) bool { return true } if prometheusCompatibleResponse { - fmt.Fprintf(w, `{"status":"success","data":{"name":%q}}`, snapshotPath) + fmt.Fprintf(w, `{"status":"success","data":{"name":%s}}`, stringsutil.JSONString(snapshotPath)) } else { - fmt.Fprintf(w, `{"status":"ok","snapshot":%q}`, snapshotPath) + fmt.Fprintf(w, `{"status":"ok","snapshot":%s}`, stringsutil.JSONString(snapshotPath)) } return true case "/list": @@ -636,5 +637,6 @@ func writeStorageMetrics(w io.Writer, strg *storage.Storage) { func jsonResponseError(w http.ResponseWriter, err error) { logger.Errorf("%s", err) w.WriteHeader(http.StatusInternalServerError) - fmt.Fprintf(w, `{"status":"error","msg":%q}`, err) + errStr := err.Error() + fmt.Fprintf(w, `{"status":"error","msg":%s}`, stringsutil.JSONString(errStr)) } diff --git a/lib/httpserver/httpserver.go b/lib/httpserver/httpserver.go index c3078d72d..fe64ad03e 100644 --- a/lib/httpserver/httpserver.go +++ b/lib/httpserver/httpserver.go @@ -20,14 +20,16 @@ import ( "sync/atomic" "time" + "github.com/VictoriaMetrics/metrics" + "github.com/klauspost/compress/gzhttp" + "github.com/valyala/fastrand" + "github.com/VictoriaMetrics/VictoriaMetrics/lib/appmetrics" "github.com/VictoriaMetrics/VictoriaMetrics/lib/fasttime" "github.com/VictoriaMetrics/VictoriaMetrics/lib/flagutil" "github.com/VictoriaMetrics/VictoriaMetrics/lib/logger" "github.com/VictoriaMetrics/VictoriaMetrics/lib/netutil" - "github.com/VictoriaMetrics/metrics" - "github.com/klauspost/compress/gzhttp" - "github.com/valyala/fastrand" + "github.com/VictoriaMetrics/VictoriaMetrics/lib/stringsutil" ) var ( @@ -533,7 +535,7 @@ func GetQuotedRemoteAddr(r *http.Request) string { remoteAddr += ", X-Forwarded-For: " + addr } // quote remoteAddr and X-Forwarded-For, since they may contain untrusted input - return strconv.Quote(remoteAddr) + return stringsutil.JSONString(remoteAddr) } type responseWriterWithAbort struct { diff --git a/lib/promscrape/config_test.go b/lib/promscrape/config_test.go index bd4a39c44..369053e18 100644 --- a/lib/promscrape/config_test.go +++ b/lib/promscrape/config_test.go @@ -3,7 +3,6 @@ package promscrape import ( "fmt" "reflect" - "strconv" "strings" "testing" "time" @@ -13,6 +12,7 @@ import ( "github.com/VictoriaMetrics/VictoriaMetrics/lib/promscrape/discovery/gce" "github.com/VictoriaMetrics/VictoriaMetrics/lib/promutils" "github.com/VictoriaMetrics/VictoriaMetrics/lib/proxy" + "github.com/VictoriaMetrics/VictoriaMetrics/lib/stringsutil" ) func TestMergeLabels(t *testing.T) { @@ -432,7 +432,7 @@ scrape_configs: // String returns human-readable representation for sw. func (sw *ScrapeWork) String() string { - return strconv.Quote(sw.key()) + return stringsutil.JSONString(sw.key()) } func TestGetFileSDScrapeWorkSuccess(t *testing.T) { diff --git a/lib/promscrape/targetstatus.go b/lib/promscrape/targetstatus.go index 2a74a3f69..742c7e305 100644 --- a/lib/promscrape/targetstatus.go +++ b/lib/promscrape/targetstatus.go @@ -19,6 +19,7 @@ import ( "github.com/VictoriaMetrics/VictoriaMetrics/lib/logger" "github.com/VictoriaMetrics/VictoriaMetrics/lib/promrelabel" "github.com/VictoriaMetrics/VictoriaMetrics/lib/promutils" + "github.com/VictoriaMetrics/VictoriaMetrics/lib/stringsutil" ) var maxDroppedTargets = flag.Int("promscrape.maxDroppedTargets", 10000, "The maximum number of droppedTargets to show at /api/v1/targets page. "+ @@ -261,21 +262,21 @@ func (tsm *targetStatusMap) WriteActiveTargetsJSON(w io.Writer) { writeLabelsJSON(w, ts.sw.Config.OriginalLabels) fmt.Fprintf(w, `,"labels":`) writeLabelsJSON(w, ts.sw.Config.Labels) - fmt.Fprintf(w, `,"scrapePool":%q`, ts.sw.Config.Job()) - fmt.Fprintf(w, `,"scrapeUrl":%q`, ts.sw.Config.ScrapeURL) + fmt.Fprintf(w, `,"scrapePool":%s`, stringsutil.JSONString(ts.sw.Config.Job())) + fmt.Fprintf(w, `,"scrapeUrl":%s`, stringsutil.JSONString(ts.sw.Config.ScrapeURL)) errMsg := "" if ts.err != nil { errMsg = ts.err.Error() } - fmt.Fprintf(w, `,"lastError":%q`, errMsg) - fmt.Fprintf(w, `,"lastScrape":%q`, time.Unix(ts.scrapeTime/1000, (ts.scrapeTime%1000)*1e6).Format(time.RFC3339Nano)) + fmt.Fprintf(w, `,"lastError":%s`, stringsutil.JSONString(errMsg)) + fmt.Fprintf(w, `,"lastScrape":"%s"`, time.Unix(ts.scrapeTime/1000, (ts.scrapeTime%1000)*1e6).Format(time.RFC3339Nano)) fmt.Fprintf(w, `,"lastScrapeDuration":%g`, (time.Millisecond * time.Duration(ts.scrapeDuration)).Seconds()) fmt.Fprintf(w, `,"lastSamplesScraped":%d`, ts.samplesScraped) state := "up" if !ts.up { state = "down" } - fmt.Fprintf(w, `,"health":%q}`, state) + fmt.Fprintf(w, `,"health":%s}`, stringsutil.JSONString(state)) if i+1 < len(tss) { fmt.Fprintf(w, `,`) } @@ -287,7 +288,7 @@ func writeLabelsJSON(w io.Writer, labels *promutils.Labels) { fmt.Fprintf(w, `{`) labelsList := labels.GetLabels() for i, label := range labelsList { - fmt.Fprintf(w, "%q:%q", label.Name, label.Value) + fmt.Fprintf(w, "%s:%s", stringsutil.JSONString(label.Name), stringsutil.JSONString(label.Value)) if i+1 < len(labelsList) { fmt.Fprintf(w, `,`) } diff --git a/lib/protoparser/opentelemetry/firehose/http.go b/lib/protoparser/opentelemetry/firehose/http.go index a222e7279..4128a661d 100644 --- a/lib/protoparser/opentelemetry/firehose/http.go +++ b/lib/protoparser/opentelemetry/firehose/http.go @@ -4,6 +4,8 @@ import ( "fmt" "net/http" "time" + + "github.com/VictoriaMetrics/VictoriaMetrics/lib/stringsutil" ) // WriteSuccessResponse writes success response for AWS Firehose request. @@ -17,7 +19,7 @@ func WriteSuccessResponse(w http.ResponseWriter, r *http.Request) { return } - body := fmt.Sprintf(`{"requestId":%q,"timestamp":%d}`, requestID, time.Now().UnixMilli()) + body := fmt.Sprintf(`{"requestId":%s,"timestamp":%d}`, stringsutil.JSONString(requestID), time.Now().UnixMilli()) h := w.Header() h.Set("Content-Type", "application/json") diff --git a/lib/streamaggr/streamaggr_timing_test.go b/lib/streamaggr/streamaggr_timing_test.go index 530ac9d6b..bb0d98afa 100644 --- a/lib/streamaggr/streamaggr_timing_test.go +++ b/lib/streamaggr/streamaggr_timing_test.go @@ -2,12 +2,12 @@ package streamaggr import ( "fmt" - "strconv" "strings" "testing" "time" "github.com/VictoriaMetrics/VictoriaMetrics/lib/prompbmarshal" + "github.com/VictoriaMetrics/VictoriaMetrics/lib/stringsutil" ) var benchOutputs = []string{ @@ -78,7 +78,7 @@ func benchmarkAggregatorsPush(b *testing.B, output string) { func newBenchAggregators(outputs []string, pushFunc PushFunc) *Aggregators { outputsQuoted := make([]string, len(outputs)) for i := range outputs { - outputsQuoted[i] = strconv.Quote(outputs[i]) + outputsQuoted[i] = stringsutil.JSONString(outputs[i]) } config := fmt.Sprintf(` - match: http_requests_total diff --git a/lib/stringsutil/json.go b/lib/stringsutil/json.go new file mode 100644 index 000000000..eb40def6f --- /dev/null +++ b/lib/stringsutil/json.go @@ -0,0 +1,10 @@ +package stringsutil + +import ( + "github.com/valyala/quicktemplate" +) + +// JSONString returns JSON-quoted s. +func JSONString(s string) string { + return string(quicktemplate.AppendJSONString(nil, s, true)) +} diff --git a/lib/stringsutil/json_test.go b/lib/stringsutil/json_test.go new file mode 100644 index 000000000..f2939b070 --- /dev/null +++ b/lib/stringsutil/json_test.go @@ -0,0 +1,20 @@ +package stringsutil + +import ( + "testing" +) + +func TestJSONString(t *testing.T) { + f := func(s, resultExpected string) { + t.Helper() + + result := JSONString(s) + if result != resultExpected { + t.Fatalf("unexpected result\ngot\n%s\nwant\n%s", result, resultExpected) + } + } + + f(``, `""`) + f(`foo`, `"foo"`) + f("\n\b\f\t\"acЫВА'\\", `"\n\b\f\t\"acЫВА\u0027\\"`) +}