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

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

Open
adamdecaf opened this issue Sep 14, 2018 · 3 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) FeatureRequest
Milestone

Comments

@adamdecaf
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
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. ;)

@adonovan adonovan added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Apr 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) FeatureRequest
Projects
None yet
Development

No branches or pull requests

5 participants