panic in context.go

[zema1]

I use martian in my private project and it saved me great efforts to build my own mitm proxy.
But I encounter a panic in context.go. The panic stack are as follows:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x28 pc=0x16088f9]

goroutine 2954 [running]:
sync.(*RWMutex).RLock(...)
        /usr/local/go/src/sync/rwmutex.go:48
github.com/google/martian.(*Context).Get(0x0, 0x1bdba16, 0x11, 0x0, 0x0, 0x1ac6900)
        /go/pkg/mod/github.com/google/martian@v2.1.0+incompatible/context.go:190 +0x39
github.com/google/martian/header.(*ViaModifier).ModifyResponse(0xc00000c800, 0xc0024b2e10, 0xc0001bc030, 0x1)
        /go/pkg/mod/github.com/google/martian@v2.1.0+incompatible/header/via_modifier.go:79 +0x59
github.com/google/martian/fifo.(*Group).ModifyResponse(0xc0001bc000, 0xc0024b2e10, 0x0, 0x0)
        /go/pkg/mod/github.com/google/martian@v2.1.0+incompatible/fifo/fifo_group.go:132 +0xe8
github.com/google/martian.(*Proxy).handle(0xc0013f9500, 0xc0012fadc0, 0x1d6ed60, 0xc00000e030, 0xc00007c460, 0x0, 0x0)
        /go/pkg/mod/github.com/google/martian@v2.1.0+incompatible/proxy.go:465 +0x78f
github.com/google/martian.(*Proxy).handleLoop(0xc0013f9500, 0x1d6ed60, 0xc00000e030)
        /go/pkg/mod/github.com/google/martian@v2.1.0+incompatible/proxy.go:243 +0x396
created by github.com/google/martian.(*Proxy).Serve
        /go/pkg/mod/github.com/google/martian@v2.1.0+incompatible/proxy.go:216 +0x313

That's was the completely call stack and I can't reproduce the error stably。I try to trace the error,
it seems that NewContext method in context.go returned a nil Context, which caused the nil pointer reference panic. But I have no idea why the ctxs has no desired Request。

ctxs  = make(map[*http.Request]*Context)

I use martian in my project like that:

	stack := fifo.NewGroup()


	// remove hop by hop headers
	hbhm := header.NewHopByHopModifier()
	stack.AddRequestModifier(hbhm)
	// try to fix error packet frame
	stack.AddRequestModifier(header.NewBadFramingModifier())

	// add via and x-forwarded-for headers to obey the HTTP proxy RFC
	if p.conf.ProxyHeader.XForwarded {
		stack.AddRequestModifier(header.NewForwardedModifier())
	}

	if p.conf.ProxyHeader.Via != "" {
		vm := header.NewViaModifier(p.conf.ProxyHeader.Via)
		stack.AddRequestModifier(vm)
		stack.AddResponseModifier(vm)
	}

	stack.AddResponseModifier(hbhm)
	return stack, nil

Thanks for any response.

[bramhaghosh]

Ok, this looks like it's happening in the via_modifier when it detects a request loop - the call to context.Get from ModifyResponse is incorrect (it does not return an error, but an ok value)

I should have a fix for you shortly.

[zema1]

I finally found out the real reason behind the panic. response.Request sometimes will be changed by go transport, which caused the request is missing in ctxs then the panic occured if we try to get the Context related to the request.

req1 := http.NewRequest(...)
resp, err := 	http.DefaultTransport.RoundTrip(req1)

// sometimes resp.Request != req1

I try to figure out when the request will be changed and found that if the roundtrip failed it will retry in some cases. But the request will be recreated before retry. Checking the code at https://github.com/golang/go/blob/249c85d3aab2ad2d0bcbf36efe606fdd66f25c72/src/net/http/transport.go#L582 , you will see what I mean.

I make a pr to solve the bug. It simply add a line after roundtrip.

res.Request = req

[zema1]

#308

[bramhaghosh]

Oh ok - this seems like it should work fine. Just want to note that the net/http docs say about Response.Request:

// This is only populated for Client requests.

But I don't see how setting it here would cause any problems.

@admtnnr @hueich can you think of any reason that setting this here would be problematic?