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

net/http: Request.ParseMultipartForm doesn't read all the body #32935

Open
Tevic opened this issue Jul 4, 2019 · 7 comments · May be fixed by #33048
Milestone

Comments

@Tevic
Copy link
Contributor

@Tevic Tevic commented Jul 4, 2019

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

$ go version
tested in go1.12.6 and go1.13beta1

Does this issue reproduce with the latest release?

yes

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

Ubuntu 18.04.2 LTS

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/tevic/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/tevic/Work/WorkSpace/Code/GoProjs"
GOPROXY=""
GORACE=""
GOROOT="/home/tevic/Work/RunTime/Go"
GOTMPDIR=""
GOTOOLDIR="/home/tevic/Work/RunTime/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-build281284214=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I send a multipart form request with a trailer, and the server side will parse the form use ParseMultipartForm,after that i get trailer from request. But sometimes it doesn't show up.
This is my example. Two test servers are for comparison. srvReadBody works like a charm while srvParseForm don't.

package main

import (
	"bytes"
	"errors"
	"fmt"
	"hash"
	"io"
	"io/ioutil"
	"log"
	"mime/multipart"
	"net/http"
	"net/http/httptest"
)

type eofReaderFunc func()

func (f eofReaderFunc) Read(p []byte) (n int, err error) {
	f()
	return 0, io.EOF
}

func main() {
	var testTrailer = "TestTrailer"
	srvReadBody := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		_, err := io.Copy(ioutil.Discard, r.Body)
		if err != nil {
			log.Println(err)
			return
		}
		if r.Trailer.Get(testTrailer) == "" {
			fmt.Println("srvReadBody: Trailer is empty.")
		}
	}))

	srvParseForm := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		err := r.ParseMultipartForm(1024 * 1024 * 16)
		if err != nil {
			log.Println(err)
			return
		}
		if r.Trailer.Get(testTrailer) == "" {
			fmt.Println("srvParseForm: Trailer is empty.")
		}
	}))

	var sendRequestWithTrailer = func(addr string) {

		buf := make([]byte, 1024)
		body := &bytes.Buffer{}
		writer := multipart.NewWriter(body)
		part, err := writer.CreateFormFile("file", "testFile")
		if err != nil {
			log.Println(err)
			return
		}
		_, err = io.Copy(part, bytes.NewReader(buf))
		if err != nil {
			log.Fatal(err)
		}
		err = writer.Close()
		if err != nil {
			log.Println(err)
			return
		}

		var req *http.Request
		req, err = http.NewRequest("POST", addr, io.MultiReader(body, eofReaderFunc(func() {
			req.Trailer.Set(testTrailer, testTrailer)
		})))
		if err != nil {
			log.Println(err)
			return
		}
		req.Header.Set("Content-Type", writer.FormDataContentType())
		req.Trailer = http.Header{testTrailer: nil}
		resp, err := http.DefaultClient.Do(req)
		if err != nil {
			log.Println(err)
			return
		}
		defer resp.Body.Close()
	}

	for i := 0; i < 100; i++ {
		sendRequestWithTrailer(srvReadBody.URL)
		sendRequestWithTrailer(srvParseForm.URL)
	}
}

What did you expect to see?

r.Trailer.Get(testTrailer) != ""

What did you see instead?

Sometimes r.Trailer.Get(testTrailer) == ""
The code output like:

srvParseForm: Trailer is empty.
srvParseForm: Trailer is empty.
srvParseForm: Trailer is empty.
srvParseForm: Trailer is empty.
srvParseForm: Trailer is empty.
srvParseForm: Trailer is empty.
srvParseForm: Trailer is empty.
srvParseForm: Trailer is empty.
srvParseForm: Trailer is empty.
srvParseForm: Trailer is empty.
srvParseForm: Trailer is empty.
srvParseForm: Trailer is empty.
srvParseForm: Trailer is empty.
srvParseForm: Trailer is empty.
srvParseForm: Trailer is empty.

I've trace the code and the problem is due to this line. When isFinalBoundary match the condition the body may have not been completely read.

if r.isFinalBoundary(line) {

@dmitshur dmitshur changed the title net/http/request: ParseMultipartForm doesn't read all the body net/http: Request.ParseMultipartForm doesn't read all the body Jul 4, 2019
@Tevic Tevic mentioned this issue Jul 6, 2019
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jul 8, 2019

The example you've provided runs without error in the Playground. Can you provide a more deterministic example?

@bcmills bcmills added this to the Go1.14 milestone Jul 8, 2019
@Tevic

This comment has been minimized.

Copy link
Contributor Author

@Tevic Tevic commented Jul 9, 2019

The example you've provided runs without error in the Playground. Can you provide a more deterministic example?

Yes, i can't figure out why. But the problem do occur on my windows 10 pc and ubuntu 18.04 LTS with go1.12.7.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jul 9, 2019

The playground runs with GOMAXPROCS=1, so it produces different interleavings of goroutines than a typical multi-core desktop machine.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jul 9, 2019

(You may be able to amplify the raciness in the playground by inserting calls to runtime.Gosched, which should be equivalent to a no-op.)

blindpirate added a commit to blindpirate/go that referenced this issue Jul 11, 2019
Fixes golang#32935
Previously, when ParseMultipartForm reaches final boundary line, there
might still be contents (e.g. Trailer) left unread after HTTP request body.
This commit fixes it by reading once more upon final boundary line.
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jul 11, 2019

Change https://golang.org/cl/185637 mentions this issue: mime/multipart: make sure all contents are read from multipart form

@blindpirate

This comment has been minimized.

Copy link

@blindpirate blindpirate commented Jul 11, 2019

I spent a little time on it. This is due to misbehavior of multipart.NextPart. When ParseMultipartForm reaches final boundary line, there might still be bytes left unread:

(HTTP Request Body)

25
This is the data in the first chunk

----- The reader has finished reading the bytes above, but the bytes below haven't been read yet
0

(The following Trailer after HTTP request body with chunked encoding)

When this happens, chunkedReader stops reading the rest part but not reports an EOF, thus this line isn't executed. In this case, multipart.NextPart returns without finishing reading all bytes in HTTP body.

I created a PR by reading once more upon final boundary line to make sure all bytes in HTTP request body are successfully read.

@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@odeke-em

This comment has been minimized.

Copy link
Member

@odeke-em odeke-em commented Oct 15, 2019

Thank you for the report and for the PR @Tevic! So even a

   peek, err := r.bufReader.Peek(1)

instead of a whole line seems to solve the problem. Could you please perhaps explain why a peek which isn't meant to advance the reader works just as well as the entire r.bufReader.ReadSlice('\n') that you have in your CL? Also doesn't that extra slice read pose a threat of request smuggling?

To better understand the problem, could you please perhaps modify this raw request write to show how not doing that read in mime/multipart on the boundary produces the error?
Please see https://play.golang.org/p/O3KTaTgTH9n and inlined below

package main

import (
	"bufio"
	"fmt"
	"log"
	"net"
	"net/http"
	"net/http/httptest"
	"net/http/httputil"
	"net/url"
)

func main() {
	testTrailer := "TestTrailer"
	cst := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		err := r.ParseMultipartForm(1024 * 16)
		if err != nil {
			log.Fatal("parsing form: ", err)
		}
		if r.Trailer.Get(testTrailer) == "" {
			log.Fatalf("expected Trailer not found: Trailer is empty.")
		}
		fmt.Printf("r.Trailer: %#v\n", r.Trailer)
		w.Write([]byte("Aloha, World!"))
	}))
	defer cst.Close()

	u, _ := url.Parse(cst.URL)

	conn, err := net.Dial("tcp", u.Host)
	if err != nil {
		log.Fatalf("Failed to dial to host: %v", err)
	}
	defer conn.Close()

	reqStr := "" +
		"POST / HTTP/1.1\r\nHost: 127.0.0.1:59265\r\nTransfer-Encoding: chunked\r\nAccept-Encoding: gzip\r\n" +
		"Content-Type: multipart/form-data; boundary=313092f87349b5232f6b6b5bef3f1ba2897ac3b3b8046bbeecc7abb42951\r\n" +
		"User-Agent: Go-http-client/1.1\r\n\r\n130\r\n--313092f87349b5232f6b6b5bef3f1ba2897ac3b3b8046bbeecc7abb42951\r\n" +
		"Content-Disposition: form-data; name=\"file\"; filename=\"testFile\"\r\nContent-Type: application/octet-stream\r\n\r\n" +
		"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
		"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
		"\x00\x00\x00\x00\r\n--313092f87349b5232f6b6b5bef3f1ba2897ac3b3b8046bbeecc7abb42951--\r\n\r\n0\r\n" +
		"TestTrailer: FooBar\r\nTrailer2: Hello\r\n\r\n"

	if _, err := conn.Write([]byte(reqStr)); err != nil {
		log.Fatalf("Failed to write request: %v", err)
	}
	br := bufio.NewReader(conn)
	res, err := http.ReadResponse(br, nil)
	if err != nil {
		log.Fatalf("Failed to read response: %v", err)
	}
	blob, _ := httputil.DumpResponse(res, true)
	println(string(blob))
}

if you can concretely show the body that's sent but cause the server's ParseMultipartForm to trip out we can then move on with the CL, thank you!

/cc @bradfitz

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.