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

metadata.AppendToOutgoingContext may cause the metadata in parent context to be modified #1925

Closed
crowhou opened this issue Mar 17, 2018 · 1 comment
Assignees

Comments

@crowhou
Copy link

crowhou commented Mar 17, 2018

What version of gRPC are you using?

master branch:  fa28bef9392c6c3e28e75389d8be8a6797561f57

What version of Go are you using (go version)?

go version go1.10 darwin/amd64

What operating system (Linux, Windows, …) and version?

macOS High Sierra 
Version 10.13.3

What did you do?

package main

import (
	"context"
	"fmt"
	"reflect"
	"testing"

	"google.golang.org/grpc/metadata"
)

func TestAppendToOutgoingContext(t *testing.T) {
	var ctx = context.Background()

	var ctx1 context.Context
	var ctx2 context.Context

	for i := 0; i < 100; i = i + 2 {
		ctx1 = metadata.AppendToOutgoingContext(ctx, fmt.Sprintf("k1-%d", i), fmt.Sprintf("v1-%d", i+1))
		ctx2 = metadata.AppendToOutgoingContext(ctx, fmt.Sprintf("k2-%d", i), fmt.Sprintf("v2-%d", i+1))

		md1, _ := metadata.FromOutgoingContext(ctx1)
		md2, _ := metadata.FromOutgoingContext(ctx2)

		if reflect.DeepEqual(md1, md2) {
			panic(fmt.Sprintf("md1:(%v) and md2:(%v) should not be equal", md1, md2))
		}

		ctx = ctx1
	}
}

This test case failed with:

--- FAIL: TestAppendToOutgoingContext (0.00s)
panic: md1:(map[k1-0:[v1-1] k1-2:[v1-3] k1-4:[v1-5] k2-6:[v2-7]]) and md2:(map[k1-0:[v1-1] k1-2:[v1-3] k1-4:[v1-5] k2-6:[v2-7]]) should not be equal [recovered]

goroutine 5 [running]:
testing.tRunner.func1(0xc4200a20f0)
	GOROOT/src/testing/testing.go:711 +0x2d2
panic(0x110c5c0, 0xc42000e920)
	GOROOT/src/runtime/panic.go:491 +0x283
tests/grpc-go.TestAppendToOutgoingContext(0xc4200a20f0)
	tests/grpc-go/metadata_test.go:26 +0x691
testing.tRunner(0xc4200a20f0, 0x1144398)
	GOROOT/src/testing/testing.go:746 +0xd0
created by testing.(*T).Run
	GOROOT/src/testing/testing.go:789 +0x2de

What did you expect to see?

Unit test TestAppendToOutgoingContext pass.

What did you see instead?

https://github.com/grpc/grpc-go/blob/fa28bef9392c6c3e28e75389d8be8a6797561f57/metadata/metadata.go#L134
rawMD{md: md.md, added: append(md.added, kv)}

This may cause md.added in parent ctx to be modified.

@dfawley
Copy link
Member

dfawley commented Mar 19, 2018

Thanks for filing the issue. As you note, it's improper to append directly to the slice, as that can modify the parent slice.

@dfawley dfawley self-assigned this Mar 19, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Sep 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants