lib/promutils: properly return error when incorrect Prometheus label names are passed to NewLabelsFromString()

Updates https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4284
See also https://github.com/VictoriaMetrics/VictoriaMetrics/pull/4304

docs/CHANGELOG.md

@@ -61,6 +61,7 @@ The following tip changes can be tested by building VictoriaMetrics components f
 * BUGFIX: change the max allowed value for `-memory.allowedPercent` from 100 to 200. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4171).
 * BUGFIX: properly limit the number of [OpenTSDB HTTP](https://docs.victoriametrics.com/#sending-opentsdb-data-via-http-apiput-requests) concurrent requests specified via `-maxConcurrentInserts` command-line flag. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4204). Thanks to @zouxiang1993 for [the fix](https://github.com/VictoriaMetrics/VictoriaMetrics/pull/4208).
 * BUGFIX: do not ignore trailing empty field in CSV lines when [importing data in CSV format](https://docs.victoriametrics.com/#how-to-import-csv-data). See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4048).
+* BUGFIX: disallow `"` chars when parsing Prometheus label names, since they aren't allowed by [Prometheus text exposition format](https://github.com/prometheus/docs/blob/main/content/docs/instrumenting/exposition_formats.md#text-format-example). Previously this could result in silent incorrect parsing of incorrect Prometheus labels such as `foo{"bar"="baz"}` or `{foo:"bar",baz="aaa"}`. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4284).
 * BUGFIX: [VictoriaMetrics cluster](https://docs.victoriametrics.com/Cluster-VictoriaMetrics.html): prevent from possible panic when the number of vmstorage nodes increases when [automatic vmstorage discovery](https://docs.victoriametrics.com/Cluster-VictoriaMetrics.html#automatic-vmstorage-discovery) is enabled.
 * BUGFIX: [MetricsQL](https://docs.victoriametrics.com/MetricsQL.html): fix a panic when the duration in the query contains uppercase `M` suffix. Such a suffix isn't allowed to use in durations, since it clashes with `a million` suffix, e.g. it isn't clear whether `rate(metric[5M])` means rate over 5 minutes, 5 months or 5 million seconds. See [this](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3589) and [this](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4120) issues.
 * BUGFIX: [vmagent](https://docs.victoriametrics.com/vmagent.html): properly handle the `vm_promscrape_config_last_reload_successful` metric after config reload. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4260).

lib/promutils/labels_test.go

@@ -194,3 +194,52 @@ func TestLabels_Set(t *testing.T) {
 	f(`http_request_total{a="b"}`, `a`, `c`, `{__name__="http_request_total",a="c"}`)
 	f(`http_request_total{a="b"}`, `ip`, `127.0.0.1`, `{__name__="http_request_total",a="b",ip="127.0.0.1"}`)
 }
+
+func TestNewLabelsFromStringSuccess(t *testing.T) {
+	f := func(s, resultExpected string) {
+		t.Helper()
+		labels, err := NewLabelsFromString(s)
+		if err != nil {
+			t.Fatalf("unexpected error: %s", err)
+		}
+		result := labels.String()
+		if result != resultExpected {
+			t.Fatalf("unexpected result;\ngot\n%s\nwant\n%s", result, resultExpected)
+		}
+	}
+
+	f("{}", "{}")
+	f("foo", `{__name__="foo"}`)
+	f(`foo{bar="baz"}`, `{__name__="foo",bar="baz"}`)
+	f(`foo {bar="baz", a="b"}`, `{__name__="foo",bar="baz",a="b"}`)
+	f(`{foo="bar", baz="a"}`, `{foo="bar",baz="a"}`)
+	f(`{__name__="aaa"}`, `{__name__="aaa"}`)
+	f(`{__name__="abc",de="fg"}`, `{__name__="abc",de="fg"}`)
+}
+
+func TestNewLabelsFromStringFailure(t *testing.T) {
+	f := func(s string) {
+		t.Helper()
+		labels, err := NewLabelsFromString(s)
+		if labels != nil {
+			t.Fatalf("unexpected non-nil labels: %s", labels)
+		}
+		if err == nil {
+			t.Fatalf("expecting non-nil error")
+		}
+	}
+
+	f("")
+	f("foo bar")
+	f(`foo{`)
+	f(`foo{bar`)
+	f(`foo{bar=`)
+	f(`foo{bar="`)
+	f(`foo{bar="baz`)
+	f(`foo{bar="baz"`)
+	f(`foo{bar="baz",`)
+	f(`foo{"bar"="baz"}`)
+	f(`{"bar":"baz"}`)
+	f(`{bar:"baz"}`)
+	f(`{bar=~"baz"}`)
+}

lib/protoparser/prometheus/parser.go

@@ -249,6 +249,9 @@ func unmarshalTags(dst []Tag, s string, noEscapes bool) (string, []Tag, error) {
 			return s, dst, fmt.Errorf("missing value for tag %q", s)
 		}
 		key := skipTrailingWhitespace(s[:n])
+		if strings.IndexByte(key, '"') >= 0 {
+			return s, dst, fmt.Errorf("tag key %q cannot contain double quotes", key)
+		}
 		s = skipLeadingWhitespace(s[n+1:])
 		if len(s) == 0 || s[0] != '"' {
 			return s, dst, fmt.Errorf("expecting quoted value for tag %q; got %q", key, s)

lib/protoparser/prometheus/parser_test.go

@@ -216,6 +216,11 @@ func TestRowsUnmarshalFailure(t *testing.T) {
 	f(`a {foo  ="bar" , `)
 	f(`a {foo  ="bar" , baz } 2`)
 
+	// invalid tags - see https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4284
+	f(`a{"__name__":"upsd_time_left_ns","host":"myhost", status_OB="true"} 12`)
+	f(`a{host:"myhost"} 12`)
+	f(`a{host:"myhost",foo="bar"} 12`)
+
 	// empty metric name
 	f(`{foo="bar"}`)