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

cmd/vet: check for http.ResponseWriter WriteHeader calls after body has been written #27668

Open
adamdecaf opened this Issue Sep 14, 2018 · 3 comments

Comments

Projects
None yet
4 participants
@adamdecaf
Contributor

adamdecaf commented Sep 14, 2018

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

$ go version
go version go1.11 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/adam/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/adam/code"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build369596792=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Consider the following code:

package main

import (
        "encoding/json"
        "fmt"
        "net/http"
)

func main() {
        http.HandleFunc("/ping", func (w http.ResponseWriter, r *http.Request) {
                type response struct {
                        Error error `json:"error"`
                }
                if err := json.NewEncoder(w).Encode(response{nil}); err != nil {
                        fmt.Println(err)
                }
                w.Header().Set("Content-Type", "application/json; charset=utf-8")
        })

        go http.ListenAndServe(":6060", nil)

        resp, err := http.Get("http://localhost:6060")
        if err != nil {
                panic(err)
        }
        fmt.Println(resp.Header.Get("Content-Type"))
}

This code outputs:

$ go run /tmp/vet/main.go
text/plain; charset=utf-8

Comments

It would be nice for vet to be aware of this common mistake and be able to alert the developer to an easy fix. The documentation for http.ResponseWriter describes all the details, but that's often missed.

@meirf

This comment has been minimized.

Member

meirf commented Sep 15, 2018

@adamdecaf this new vet logic would need to ignore the case where trailers are being set?

@meirf meirf added this to the Unplanned milestone Sep 15, 2018

@dmitshur

This comment has been minimized.

Member

dmitshur commented Sep 17, 2018

/cc @alandonovan @josharian @mvdan per owners.

Also /cc @dominikh in case this is one of the checks you've considered previously for staticcheck.

@alandonovan

This comment has been minimized.

Contributor

alandonovan commented Sep 17, 2018

I quickly prototyped this using the approach of the nilness checker, looking for a call to w.Header().Set() dominated by a conversion of w to io.Writer. You could go further by identifying other functions that "may write to w" while it is still a ResponseWriter.

func run(unit *analysis.Unit) error {
	ssainput := unit.Inputs[buildssa.Analysis].(*buildssa.SSA)

	// Skip the analysis unless the package directly imports net/http.
	// (In theory problems could occur even in packages that depend
	// on net/http only indirectly, but that's quite unlikely.)
	var httpPkg *types.Package
	for _, imp := range unit.Pkg.Imports() {
		if imp.Path() == "net/http" {
			httpPkg = imp
			break
		}
	}
	if httpPkg == nil {
		return nil // doesn't import net/http
	}

	// Find net/http objects.
	responseWriterType := httpPkg.Scope().Lookup("ResponseWriter")
	headerType := httpPkg.Scope().Lookup("Header")
	headerSetMethod, _, _ := types.LookupFieldOrMethod(headerType.Type(), false, nil, "Set")
	if headerSetMethod == nil {
		return fmt.Errorf("internal error: no (net/http.Header).Set method")
	}

	// isWriter reports whether t satisfies io.Writer.
	isWriter := func(t types.Type) bool {
		obj, _, _ := types.LookupFieldOrMethod(t, false, nil, "Write")
		return obj != nil
	}

	for _, fn := range ssainput.SrcFuncs {
		// visit visits reachable blocks of the CFG in dominance
		// order, maintaining a stack of dominating facts.
		//
		// The stack records values of type http.ResponseWriter
		// that were converted to io.Writer. This is taken as
		// a proxy for writing the HTTP response body.
		type fact struct{ ssa.Value }

		seen := make([]bool, len(fn.Blocks)) // seen[i] means visit should ignore block i
		var visit func(b *ssa.BasicBlock, stack []fact)
		visit = func(b *ssa.BasicBlock, stack []fact) {
			if seen[b.Index] {
				return
			}
			seen[b.Index] = true

			for _, instr := range b.Instrs {
				switch instr := instr.(type) {
				case *ssa.ChangeInterface:
					// Sadly there's no point recording instr.Pos
					// as the conversion is invarialby implicit.
					if types.Identical(instr.X.Type(), responseWriterType.Type()) && isWriter(instr.Type()) {
						stack = append(stack, fact{instr.X})
					}

				case *ssa.Call:
					// Call to w.Header().Set()?
					if callee := instr.Common().StaticCallee(); callee != nil && callee.Object() == headerSetMethod {
						hdr := instr.Common().Args[0]
						if headerCall, ok := hdr.(*ssa.Call); ok {
							w := headerCall.Common().Value
							for _, fact := range stack {
								if fact.Value == w {
									unit.Findingf(instr.Pos(), "call to w.Header().Set() after response body written")
									break
								}
							}
						}
					}
				}
			}

			for _, d := range b.Dominees() {
				visit(d, stack)
			}
		}

		// Visit the entry block.  No need to visit fn.Recover.
		if fn.Blocks != nil {
			visit(fn.Blocks[0], make([]fact, 0, 20)) // 20 is plenty
		}
	}

	return nil
}

I've attached the log of what it found in the standard library--all tests, unsurprisingly.
'
responsewriter.txt

BTW: I hope you wrote go http.ListenAndServe(":6060", nil) only for this example, as it discards the error result. ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment