code review 185000043: cmd/gorefactor: preserve file modes.

refactor/rename: preserve file modes.

Fixes issue 9013

[adonovan, 2014-12-04 23:05:42]

Hello sameer@golang.org (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go.tools

[minux, 2014-12-05 00:10:34]

https://codereview.appspot.com/185000043/diff/40001/refactor/rename/rename.go
File refactor/rename/rename.go (right):

https://codereview.appspot.com/185000043/diff/40001/refactor/rename/rename.go...
refactor/rename/rename.go:329: // Oddly, os.OpenFile doesn't preserve all the
mode bits, hence
OpenFile is subject to umask settings, so we must do Chmod.

Why not always OpenFile with 0600, and then chmod to correct mode, thus
eliminate the vulnerability window?

https://codereview.appspot.com/185000043/diff/40001/refactor/rename/rename.go...
refactor/rename/rename.go:350: var mode os.FileMode = 0666
the 0666 default mode is too insecure (for a 000 umask).
I suggest 0660, or even 0640.

[adonovan, 2014-12-05 18:38:50]

https://codereview.appspot.com/185000043/diff/40001/refactor/rename/rename.go
File refactor/rename/rename.go (right):

https://codereview.appspot.com/185000043/diff/40001/refactor/rename/rename.go...
refactor/rename/rename.go:329: // Oddly, os.OpenFile doesn't preserve all the
mode bits, hence
On 2014/12/05 00:10:34, minux wrote:
> OpenFile is subject to umask settings, so we must do Chmod.
> 
> Why not always OpenFile with 0600, and then chmod to correct mode, thus
> eliminate the vulnerability window?

Good idea; done.

https://codereview.appspot.com/185000043/diff/40001/refactor/rename/rename.go...
refactor/rename/rename.go:350: var mode os.FileMode = 0666
On 2014/12/05 00:10:34, minux wrote:
> the 0666 default mode is too insecure (for a 000 umask).
> I suggest 0660, or even 0640.

I took this default from os.Create.

In any case, it's almost inconceivable that the default would actually be used.

[Sameer Ajmani, 2014-12-05 18:41:54]

On 2014/12/05 18:38:50, adonovan wrote:
> https://codereview.appspot.com/185000043/diff/40001/refactor/rename/rename.go
> File refactor/rename/rename.go (right):
> 
>
https://codereview.appspot.com/185000043/diff/40001/refactor/rename/rename.go...
> refactor/rename/rename.go:329: // Oddly, os.OpenFile doesn't preserve all the
> mode bits, hence
> On 2014/12/05 00:10:34, minux wrote:
> > OpenFile is subject to umask settings, so we must do Chmod.
> > 
> > Why not always OpenFile with 0600, and then chmod to correct mode, thus
> > eliminate the vulnerability window?
> 
> Good idea; done.
> 
>
https://codereview.appspot.com/185000043/diff/40001/refactor/rename/rename.go...
> refactor/rename/rename.go:350: var mode os.FileMode = 0666
> On 2014/12/05 00:10:34, minux wrote:
> > the 0666 default mode is too insecure (for a 000 umask).
> > I suggest 0660, or even 0640.
> 
> I took this default from os.Create.
> 
> In any case, it's almost inconceivable that the default would actually be
used.

LGTM

[adonovan, 2014-12-05 19:07:03]

*** Submitted as
https://code.google.com/p/go/source/detail?r=8865ff253ddb&repo=tools ***

refactor/rename: preserve file modes.

Fixes issue 9013

LGTM=sameer
R=sameer, minux
CC=golang-codereviews
https://codereview.appspot.com/185000043