code review 7799046: os/exec: fix fd leak with Std*Pipe + LookPath

os/exec: fix fd leak with Std*Pipe + LookPath

If LookPath in Command fails, sets a sticky error, and then
StdinPipe, StdoutPipe, or StderrPipe were called, those pipe
fds were never cleaned up.

Fixes issue 5071

[bradfitz, 2013-03-18 14:56:04]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[rog, 2013-03-18 16:04:18]

On 2013/03/18 14:56:04, bradfitz wrote:
> Hello mailto:golang-dev@googlegroups.com,
> 
> I'd like you to review this change to
> https://go.googlecode.com/hg/

I'm not sure this is right. this may well break code that currently does
(correctly):

   f, err := c.StdinPipe()
   if err != nil {
       return err
   }
   osf := f.(*os.File)

The underlying type may not be part of the automatically checked
Go 1 contract, but I'd argue that it is just as important
to maintain - with this change people's code will compile fine
but may crash when run.

[bradfitz, 2013-03-18 16:09:14]

I anticipated this complaint.  See the earlier patchset in that CL for
another approach.

I don't think Go 1 code assuming the concrete type is "correct", but
whatever.

Would you LGTM the other fix?


On Mon, Mar 18, 2013 at 9:04 AM, <rogpeppe@gmail.com> wrote:

> On 2013/03/18 14:56:04, bradfitz wrote:
>
>> Hello mailto:golang-dev@**googlegroups.com <golang-dev@googlegroups.com>,
>>
>
>  I'd like you to review this change to
>> https://go.googlecode.com/hg/
>>
>
> I'm not sure this is right. this may well break code that currently does
> (correctly):
>
>    f, err := c.StdinPipe()
>    if err != nil {
>        return err
>    }
>    osf := f.(*os.File)
>
> The underlying type may not be part of the automatically checked
> Go 1 contract, but I'd argue that it is just as important
> to maintain - with this change people's code will compile fine
> but may crash when run.
>
>
>
https://codereview.appspot.**com/7799046/<https://codereview.appspot.com/7799...
>

[rog, 2013-03-18 16:47:59]

On 18 March 2013 16:09, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> I anticipated this complaint.  See the earlier patchset in that CL for
> another approach.
>
> I don't think Go 1 code assuming the concrete type is "correct", but
> whatever.

It's difficult to say. By my reading of
http://tip.golang.org/doc/go1compat.html#expectations
we guarantee compatible behaviour even in this case (there's nothing
saying "you must not rely on the concrete type of a returned interface value").

> Would you LGTM the other fix?

Yes, I think that's better - os.Pipe will fail much less often than LookPath,
and although we potentially leak fds when it does, the process is probably
already in bad trouble when that's happening. That is, we have mitigated
the problem in a backwardly compatible way despite not fixing it completely.
I'm not sure we can do better at this point.

LGTM with the alternative fix.

[bradfitz, 2013-03-18 16:52:47]

*** Submitted as https://code.google.com/p/go/source/detail?r=bcb5f45aa10e ***

os/exec: fix fd leak with Std*Pipe + LookPath

If LookPath in Command fails, sets a sticky error, and then
StdinPipe, StdoutPipe, or StderrPipe were called, those pipe
fds were never cleaned up.

Fixes issue 5071

R=golang-dev, rogpeppe
CC=golang-dev
https://codereview.appspot.com/7799046