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

Tevic · 2019-07-04T05:06:01Z

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.

go/src/mime/multipart/multipart.go

Line 336 in 39b533e

if r.isFinalBoundary(line) {

The text was updated successfully, but these errors were encountered:

bcmills · 2019-07-08T18:50:10Z

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

Tevic · 2019-07-09T10:18:08Z

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 · 2019-07-09T13:54:59Z

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

bcmills · 2019-07-09T13:55:59Z

(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.)

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 · 2019-07-11T01:03:34Z

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

blindpirate · 2019-07-11T01:17:17Z

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.

odeke-em · 2019-10-15T01:39:57Z

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

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 mentioned this issue Jul 6, 2019

Http Trailer ma6174/blog#22

Open

bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 8, 2019

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

blindpirate mentioned this issue Jul 11, 2019

mime/multipart: make sure all contents are read from multipart form #33048

Closed

rsc modified the milestones: Go1.14, Backlog Oct 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Tevic commented Jul 4, 2019 •

edited

bcmills commented Jul 8, 2019

Tevic commented Jul 9, 2019

bcmills commented Jul 9, 2019

bcmills commented Jul 9, 2019

gopherbot commented Jul 11, 2019

blindpirate commented Jul 11, 2019

odeke-em commented Oct 15, 2019

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

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

Comments

Tevic commented Jul 4, 2019 • edited

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

Does this issue reproduce with the latest release?

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

What did you do?

What did you expect to see?

What did you see instead?

bcmills commented Jul 8, 2019

Tevic commented Jul 9, 2019

bcmills commented Jul 9, 2019

bcmills commented Jul 9, 2019

gopherbot commented Jul 11, 2019

blindpirate commented Jul 11, 2019

odeke-em commented Oct 15, 2019

Tevic commented Jul 4, 2019 •

edited

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

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