[go] mime/multipart: properly remove temp files in case of error
Gerrit Bot (Gerrit)
Gerrit Bot has uploaded this change for review.
mime/multipart: properly remove temp files in case of error
The new implementation properly uses the err variable defined in the
return values, instead of declaring a new local-scoped err variable.
Updates #20253
Change-Id: Ie6b9780028b12f7ea0836c13dafce869d1dd835d
GitHub-Last-Rev: 00f14107e6895bd21760671f2f967092a97f02df
GitHub-Pull-Request: golang/go#25381
---
M src/mime/multipart/formdata.go
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/src/mime/multipart/formdata.go b/src/mime/multipart/formdata.go
index 22e2c8d..ef5f083 100644
--- a/src/mime/multipart/formdata.go
+++ b/src/mime/multipart/formdata.go
@@ -42,7 +42,8 @@
// Reserve an additional 10 MB for non-file parts.
maxValueBytes := maxMemory + int64(10<<20)
for {
- p, err := r.NextPart()
+ var p *Part
+ p, err = r.NextPart()
if err == io.EOF {
break
}
@@ -60,7 +61,8 @@
if !p.hasFileName() {
// value, store as string in memory
- n, err := io.CopyN(&b, p, maxValueBytes+1)
+ var n int64
+ n, err = io.CopyN(&b, p, maxValueBytes+1)
if err != nil && err != io.EOF {
return nil, err
}
@@ -77,17 +79,20 @@
Filename: filename,
Header: p.Header,
}
- n, err := io.CopyN(&b, p, maxMemory+1)
+ var n int64
+ n, err = io.CopyN(&b, p, maxMemory+1)
if err != nil && err != io.EOF {
return nil, err
}
if n > maxMemory {
// too big, write to disk and flush buffer
- file, err := ioutil.TempFile("", "multipart-")
+ var file *os.File
+ file, err = ioutil.TempFile("", "multipart-")
if err != nil {
return nil, err
}
- size, err := io.Copy(file, io.MultiReader(&b, p))
+ var size int64
+ size, err = io.Copy(file, io.MultiReader(&b, p))
if cerr := file.Close(); err == nil {
err = cerr
}
To view, visit change 113055. To unsubscribe, or for help writing mail filters, visit settings.
Gobot Gobot (Gerrit)
Congratulations on opening your first change. Thank you for your contribution!
Next steps:
Within the next week or so, a maintainer will review your change and provide
feedback. See https://golang.org/doc/contribute.html#review for more info and
tips to get your patch through code review.
Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.
During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11, it means that this CL will be reviewed as part of the next development
cycle. See https://golang.org/s/release for more details.
Brad Fitzpatrick (Gerrit)
1 comment:
File src/mime/multipart/formdata.go:
err error) {
form := &Form{make(map[string][]string), make(map[string][]*FileHeader)}
defer func() {
if errhow about just renaming these two "err" to "reterr" and leaving the rest unchanged?
That seems less likely to break in the future, which is especially important as this change doesn't add any new test coverage either (and we didn't have tests before).
To view, visit change 113055. To unsubscribe, or for help writing mail filters, visit settings.
Brad Fitzpatrick (Gerrit)
1 comment:
err error) {
form := &Form{make(map[string][]string), make(map[string][]*FileHeader)}
defer func() {
if err
> how about just renaming these two "err" to "reterr" and leaving the rest unchanged? […]
You do know that reterr is assigned by return statements, right?
To view, visit change 113055. To unsubscribe, or for help writing mail filters, visit settings.
Peter Goetz (Gerrit)
1 comment:
err error) {
form := &Form{make(map[string][]string), make(map[string][]*FileHeader)}
defer func() {
if err
You do know that reterr is assigned by return statements, right? […]
Ha! I did indeed *not* know this. Will update the PR according to your recommendation.
To view, visit change 113055. To unsubscribe, or for help writing mail filters, visit settings.
Peter Goetz (Gerrit)
1 comment:
err error) {
form := &Form{make(map[string][]string), make(map[string][]*FileHeader)}
defer func() {
if err
Ha! I did indeed *not* know this. Will update the PR according to your recommendation.
Wait, in that case the code should work as is. My PR is not necessary. I will close it. It only makes me wonder why I get so many undeleted multipart temp files when I see "multipart: NextPart: bufio: buffer full" error messages. But it seems to be a different issue.
To view, visit change 113055. To unsubscribe, or for help writing mail filters, visit settings.
Peter Goetz (Gerrit)
1 comment:
err error) {
form := &Form{make(map[string][]string), make(map[string][]*FileHeader)}
defer func() {
if err
how about just renaming these two "err" to "reterr" and leaving the rest unchanged?
I think this will not work. The remaining code will still set "err" to a value, and the defer block will see a reterr == nil and not do the cleanup.
Agree that we'd like to cover this fix with tests. I tried adding a test, but the code is hard to test, because the cleanup cannot easily be mocked without bigger changes. I could trigger the problematic error situation though with:
body := `
--MyBoundary
[longline]--MyBoundary[longboundary]
bla
--MyBoundary--
`
body = strings.Replace(body, "[longboundary]", "MyBoundary"+strings.Repeat("B", (1<<15)), -1)
body = strings.Replace(body, "[longline]", strings.Repeat("X", (1<<15)), -1)
reader := NewReader(strings.NewReader(body), "MyBoundary"+strings.Repeat("B", (1<<15)))
reader.ReadForm(0)So unless we want these bigger changes to make the code testable, I don't see how we can add a test.
To view, visit change 113055. To unsubscribe, or for help writing mail filters, visit settings.
Brad Fitzpatrick (Gerrit)
No, the issue is still that (the inner err variables are shadowing the other one), but there's an additional issue I discovered when I went to also write a test for this.
Peter Goetz (Gerrit)
Patch Set 1:
No, the issue is still that (the inner err variables are shadowing the other one), but there's an additional issue I discovered when I went to also write a test for this.
Not sure I understand. Since all returns do something along the lines of "return nil, err", the inner err variable will be assigned to the return err variable (even though it shadowed that before) and the defer statement is happy, no? I slightly changed your example https://play.golang.org/p/GSpnvsFB9F7 and it shows this.
Would you mind giving another example?
Brad Fitzpatrick (Gerrit)
Patch Set 1:
Patch Set 1:
No, the issue is still that (the inner err variables are shadowing the other one), but there's an additional issue I discovered when I went to also write a test for this.
Not sure I understand. Since all returns do something along the lines of "return nil, err", the inner err variable will be assigned to the return err variable (even though it shadowed that before) and the defer statement is happy, no? I slightly changed your example https://play.golang.org/p/GSpnvsFB9F7 and it shows this.
Would you mind giving another example?
Sorry, I thought I remembered a naked return in this code. Yes, if there's no naked return, the change to name the return value reterr should be sufficient.
Time to write a test to demonstrate the problem you're seeing?
Peter Goetz (Gerrit)
Time to write a test to demonstrate the problem you're seeing?
The problem I was seeing was triggered by something completely different: the reason the temp files weren't deleted was that the request that did the ParseMultipartForm() was a different one than the one that the server in server.go tries to garbage collect by calling finishRequest(). Why was it a different one? Because modifying request contexts while passing them along handler chains creates request *copies*. So the lesson learnt here is probably that parsing and garbage collecting multipart-forms should be a symmetric process.
Will close the PR as there is no problem there.
Ian Lance Taylor (Gerrit)
Ian Lance Taylor abandoned this change.
To view, visit change 113055. To unsubscribe, or for help writing mail filters, visit settings.