Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(15)

Issue 6789043: code review 6789043: os/exec: fix fd leak on error

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 8 months ago by rog
Modified:
9 years, 7 months ago
Reviewers:
rsc, bradfitz
CC:
golang-dev
Visibility:
Public.

Description

os/exec: fix possible data loss and fd leak The reasonable-seeming code below could lose data and it will not terminate if the command does not exist. It can lose data because the client side of the pipe is closed after Wait returns, even if we haven't read all the data yet. It won't terminate when the command is not found because the server side of the pipe is not closed by Run if the command was not found, so the io.Copy never gets EOF. func getOutput(cmd string) (string, error) { c := exec.Command(cmd) r, err := c.StdoutPipe() if err != nil { return "", err } done := make(chan bool, 1) var buf bytes.Buffer go func() { io.Copy(&buf, r) r.Close() done <- true }() err = c.Run() <-done return buf.String(), err } This CL fixes the first issue above by making sure that the server side of the pipe is always closed when Start is called, even if Start returns an error. It fixes the second issue by not closing the client side of the pipe at all, and leaving it up to the code that calls StdoutPipe to close it for itself. It could be argued that this breaks the Go 1 contract, but I think it's a bug that needs fixing. Fixes issue 4290.

Patch Set 1 #

Patch Set 2 : diff -r 117f773ed6b0 https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r 117f773ed6b0 https://go.googlecode.com/hg/ #

Patch Set 4 : diff -r 57f70857cf91 https://go.googlecode.com/hg/ #

Patch Set 5 : diff -r 57f70857cf91 https://go.googlecode.com/hg/ #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+43 lines, -12 lines) Patch
M src/pkg/os/exec/exec.go View 1 2 3 10 chunks +2 lines, -12 lines 0 comments Download
M src/pkg/os/exec/exec_test.go View 1 2 3 4 2 chunks +41 lines, -0 lines 3 comments Download

Messages

Total messages: 16
rog
Hello bradfitz@golang.org (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
9 years, 8 months ago (2012-10-26 10:29:35 UTC) #1
rog
On 26 October 2012 11:29, <rogpeppe@gmail.com> wrote: > Related aside: > > I think it's ...
9 years, 8 months ago (2012-10-26 14:15:17 UTC) #2
bradfitz
Your CL description subject doesn't really match the CL description body and code. It's more ...
9 years, 8 months ago (2012-10-26 15:05:49 UTC) #3
rog
On 26 October 2012 16:04, Brad Fitzpatrick <bradfitz@golang.org> wrote: > Your CL description subject doesn't ...
9 years, 8 months ago (2012-10-26 15:41:28 UTC) #4
rog
PTAL. I've changed the scope of this issue to fix another closely related issue at ...
9 years, 8 months ago (2012-10-30 11:00:24 UTC) #5
rog
On 30 October 2012 11:00, <rogpeppe@gmail.com> wrote: > PTAL. > > I've changed the scope ...
9 years, 8 months ago (2012-10-30 11:44:21 UTC) #6
bradfitz
Okay, this makes more sense now with the updated CL description and tests. I wonder ...
9 years, 8 months ago (2012-10-30 11:57:38 UTC) #7
rog
On 30 October 2012 11:57, <bradfitz@golang.org> wrote: > Okay, this makes more sense now with ...
9 years, 8 months ago (2012-10-30 13:22:52 UTC) #8
rog
ping On 30 October 2012 13:22, roger peppe <rogpeppe@gmail.com> wrote: > On 30 October 2012 ...
9 years, 7 months ago (2012-11-12 19:59:03 UTC) #9
bradfitz
On Tue, Oct 30, 2012 at 6:22 AM, roger peppe <rogpeppe@gmail.com> wrote: > On 30 ...
9 years, 7 months ago (2012-11-12 23:57:35 UTC) #10
rog
On 12 November 2012 23:57, Brad Fitzpatrick <bradfitz@golang.org> wrote: > > > On Tue, Oct ...
9 years, 7 months ago (2012-11-13 11:43:31 UTC) #11
bradfitz
On Tue, Nov 13, 2012 at 3:43 AM, roger peppe <rogpeppe@gmail.com> wrote: > On 12 ...
9 years, 7 months ago (2012-11-13 15:06:17 UTC) #12
rsc
I agree that the code in the CL description is reasonable sounding, and if we ...
9 years, 7 months ago (2012-11-13 15:22:30 UTC) #13
rog
On 13 November 2012 15:22, <rsc@golang.org> wrote: > I agree that the code in the ...
9 years, 7 months ago (2012-11-14 08:43:42 UTC) #14
bradfitz
On Wed, Nov 14, 2012 at 12:43 AM, roger peppe <rogpeppe@gmail.com> wrote: > On 13 ...
9 years, 7 months ago (2012-11-14 16:59:04 UTC) #15
rog
9 years, 7 months ago (2012-11-14 18:00:46 UTC) #16
On 14 November 2012 16:59, Brad Fitzpatrick <bradfitz@golang.org> wrote:
>> Unfortunately the above doesn't work if the started process forks
>> another process in the background. In that case the Read can
>> take an indefinite time to return, and for some code (this was
>> the example that brought me up against this problem) it is
>> actually the original process exiting that is important.
>
>
> Why was the first process leaking stdout to its child?  Sounds like the real
> problem.

The first process is a shell script written by some third party - we don't
have control over it, but it's important that we finish when the script
finishes.

I suppose this may be a relatively unusual requirement,
and thus justify a special treatment. I didn't think so at the time,
but if we can't change the os/exec API at all, we can live with it.

>> What do you think about the leak-on-error issue? Should we document
>> that it's important to call Start even when a *Pipe method call
>> fails?
>
>
> If StdoutPipe or StderrPipe fails, the program's probably already pretty
> screwed (nothing good happens when you're out of fds), so I'm not too
> worried.  Especially because the finalizer will close it anyway.  I would
> err on the side less esoteric docs.

OK, seems good to me.

I'll abandon the CL and we'll write some docs/examples to point
out the pitfalls.

  cheers,
    rog.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b