Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

expfmt: TextDecoder infinite bufio Read recursion #442

Closed
abursavich opened this issue Feb 8, 2023 · 1 comment · Fixed by #443
Closed

expfmt: TextDecoder infinite bufio Read recursion #442

abursavich opened this issue Feb 8, 2023 · 1 comment · Fixed by #443

Comments

@abursavich
Copy link
Contributor

abursavich commented Feb 8, 2023

Problem

If the io.Reader passed to expfmt.NewDecoder with the expfmt.FmtText format is a *bufio.Reader, then Decoder.Decode will infinitely recurse in the bufio.(*Reader).Read function.

Explanation

expfmt.(*textDecoder).Decode calls expfmt.(*TextParser).TextToMetricFamilies passing its same io.Reader each time, which is then passed to expfmt.(*TextParser).reset, which contains the following snippet:

	if p.buf == nil {
		p.buf = bufio.NewReader(in)
	} else {
		p.buf.Reset(in)
	}

The problem is that bufio.NewReader will return the given io.Reader without wrapping it, if it happens to already be a *bufio.Reader of sufficient size. All good on the first call from Decode, but the second call ends up setting the *bufio.Reader as its own underlying io.Reader and we get an infinite recursion.

A reasonable person might argue that this is a bug in bufio, but we can still fix it in expfmt.

Repro

package main

import (
	"bufio"
	"fmt"
	"io"
	"os"
	"strings"

	dto "github.com/prometheus/client_model/go"
	"github.com/prometheus/common/expfmt"
)

const example = `
# TYPE foo gauge
foo 0
`

func main() {
	r := bufio.NewReader(strings.NewReader(example))
	dec := expfmt.NewDecoder(r, expfmt.FmtText)
	for {
		var mf dto.MetricFamily
		if err := dec.Decode(&mf); err != nil {
			if err == io.EOF {
				return
			}
			fmt.Fprintln(os.Stderr, err)
			os.Exit(1)
		}
	}
}
@abursavich
Copy link
Contributor Author

Repro of the underlying issue with just the standard library: https://go.dev/play/p/2UTNBLVR1J__E

roidelapluie added a commit to roidelapluie/common that referenced this issue Feb 20, 2023
The test case checks that a *bufio.Reader can be passed to
expfmt.NewDecoder with the FmtText format without causing an infinite
recursion. This is a regression test for the issue reported in
prometheus#442.

Signed-off-by: Julien Pivotto <roidelapluie@o11y.eu>
radek-ryckowski pushed a commit to goldmansachs/common that referenced this issue May 18, 2023
The test case checks that a *bufio.Reader can be passed to
expfmt.NewDecoder with the FmtText format without causing an infinite
recursion. This is a regression test for the issue reported in
prometheus#442.

Signed-off-by: Julien Pivotto <roidelapluie@o11y.eu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant