From e133d0d6c27f929be87bbfacdef1284ac5ef40bf Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Mon, 14 Jun 2021 12:14:27 -0400 Subject: [PATCH] Guard against negative dataLength while reading a page (#763) * Added guard code Signed-off-by: Joe Elliott * changelog Signed-off-by: Joe Elliott * test Signed-off-by: Joe Elliott --- CHANGELOG.md | 1 + integration/microservices/docker-compose.yaml | 5 +++- tempodb/encoding/v2/page.go | 4 +++ tempodb/encoding/v2/page_test.go | 25 ++++++++++++++++++- 4 files changed, 33 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a302d4327f..8a95f8b19b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ ## main / unreleased * [FEATURE] Added the ability to hedge requests with all backends [#750](https://github.com/grafana/tempo/pull/750) (@joe-elliott) +* [BUGFIX] Guard against negative dataLength [#763](https://github.com/grafana/tempo/pull/763) (@joe-elliott) ## v1.0.0 diff --git a/integration/microservices/docker-compose.yaml b/integration/microservices/docker-compose.yaml index 894361f73ff..ed92493e2e9 100644 --- a/integration/microservices/docker-compose.yaml +++ b/integration/microservices/docker-compose.yaml @@ -16,6 +16,7 @@ services: ingester-0: image: tempo:latest command: "-target=ingester -config.file=/etc/tempo.yaml" + restart: always volumes: - ./tempo.yaml:/etc/tempo.yaml ports: @@ -28,6 +29,7 @@ services: ingester-1: image: tempo:latest command: "-target=ingester -config.file=/etc/tempo.yaml" + restart: always volumes: - ./tempo.yaml:/etc/tempo.yaml ports: @@ -40,6 +42,7 @@ services: ingester-2: image: tempo:latest command: "-target=ingester -config.file=/etc/tempo.yaml" + restart: always volumes: - ./tempo.yaml:/etc/tempo.yaml ports: @@ -50,7 +53,7 @@ services: - distributor # inverted relationship here to add delay for minio minio: - image: minio/minio:RELEASE.2021-05-26T00-22-46Z + image: minio/minio:RELEASE.2021-06-14T01-29-23Z environment: - MINIO_ACCESS_KEY=tempo - MINIO_SECRET_KEY=supersecret diff --git a/tempodb/encoding/v2/page.go b/tempodb/encoding/v2/page.go index 8062686c8e1..75ccbcb0038 100644 --- a/tempodb/encoding/v2/page.go +++ b/tempodb/encoding/v2/page.go @@ -78,6 +78,10 @@ func unmarshalPageFromReader(r io.Reader, header pageHeader, buffer []byte) (*pa } dataLength := int(totalLength) - totalHeaderSize + if dataLength < 0 { + return nil, fmt.Errorf("unexpected negative dataLength unmarshalling page: %d", dataLength) + } + if cap(buffer) < dataLength { buffer = make([]byte, dataLength) } else { diff --git a/tempodb/encoding/v2/page_test.go b/tempodb/encoding/v2/page_test.go index 704adc078bb..c84382d34a7 100644 --- a/tempodb/encoding/v2/page_test.go +++ b/tempodb/encoding/v2/page_test.go @@ -16,7 +16,7 @@ type testHeader struct { func (h *testHeader) unmarshalHeader(b []byte) error { if len(b) != int(uint32Size) { - return errors.New("err") + return errors.New("testheader err") } h.field = binary.LittleEndian.Uint32(b[:uint32Size]) @@ -108,3 +108,26 @@ func TestPageMarshalToBuffer(t *testing.T) { assert.Equal(t, tc.field, page.header.(*testHeader).field) } } + +func TestCorruptHeader(t *testing.T) { + data := []byte{0x01, 0x02, 0x03} + buff := &bytes.Buffer{} + + bytesWritten, err := marshalPageToWriter(data, buff, constDataHeader) + require.NoError(t, err) + assert.Equal(t, len(data)+constDataHeader.headerLength()+int(baseHeaderSize), bytesWritten) + + buffBytes := buff.Bytes() + + // overwrite the base header with 0s + zeroHeader := bytes.Repeat([]byte{0x00}, baseHeaderSize) + copy(buffBytes, zeroHeader) + + page, err := unmarshalPageFromBytes(buffBytes, constDataHeader) + assert.Nil(t, page) + assert.EqualError(t, err, "expected data len -6 does not match actual 3") + + page, err = unmarshalPageFromReader(bytes.NewReader(buffBytes), constDataHeader, nil) + assert.Nil(t, page) + assert.EqualError(t, err, "unexpected negative dataLength unmarshalling page: -6") +}