code review 6488044: os: detect and handle console in File.Write on windows

os: detect and handle console in File.Write on windows

Fixes issue 3376.

[brainman, 2012-08-28 05:53:24]

Hello golang-dev@googlegroups.com,

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

[bsiegert, 2012-08-28 08:24:40]

I am wondering if the code in consoleWrite can be simplified by casting the byte
array to []rune somewhere. Maybe after prepending lastbits, check if the rune at
the end is incomplete, if yes cut it off, then convert the whole slice.

http://codereview.appspot.com/6488044/diff/1001/src/pkg/os/file_windows.go
File src/pkg/os/file_windows.go (right):

http://codereview.appspot.com/6488044/diff/1001/src/pkg/os/file_windows.go#ne...
src/pkg/os/file_windows.go:242: // console writes len(b) bytes to the console
File.
correct function name, s/console/consoleWrite/

http://codereview.appspot.com/6488044/diff/1001/src/pkg/os/file_windows.go#ne...
src/pkg/os/file_windows.go:244: func (f *File) consoleWrite(b []byte) (n int,
err error) {
Maybe I would call this writeConsole.

[minux1, 2012-08-28 08:27:31]

can we test it?

this is a important issue, and i think it's worthwhile even we have to
change run.bat to add a specific test for it.

[minux1, 2012-08-28 17:04:59]

I think you also need to do the same for the runtime package
(possibly in a follow-up CL)
http://tip.golang.org/src/pkg/runtime/thread_windows.c#L117

[brainman, 2012-08-29 02:33:09]

Hello golang-dev@googlegroups.com, bsiegert@gmail.com, minux.ma@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[brainman, 2012-08-29 02:33:16]

On 2012/08/28 08:24:40, bsiegert wrote:
> I am wondering if the code in consoleWrite can be simplified by casting the
byte
> array to []rune somewhere. Maybe after prepending lastbits, check if the rune
at
> the end is incomplete, if yes cut it off, then convert the whole slice.
> 

I think it is simple enough, but if you show your version, I will consider it.

On 2012/08/28 08:27:31, minux wrote:
> can we test it?
> 

I would love a test. But I can't even imaging how to do it.

> this is a important issue, and i think it's worthwhile even we have to
> change run.bat to add a specific test for it.

Again, I am open to suggestions.

On 2012/08/28 17:04:59, minux wrote:
> I think you also need to do the same for the runtime package
> (possibly in a follow-up CL)
> http://tip.golang.org/src/pkg/runtime/thread_windows.c#L117

I disagree here. I think print and println can stay as is. These are for
debugging only. I do not think complications are worth it. But feel free to
pursue your proposal one way or the other.

If we are here talking about things I didn't do, then I should mention similar
code for console input. But, again, I do not see it as a big miss, so I would
like to keep things simple until we really need it.

Alex

http://codereview.appspot.com/6488044/diff/1001/src/pkg/os/file_windows.go
File src/pkg/os/file_windows.go (right):

http://codereview.appspot.com/6488044/diff/1001/src/pkg/os/file_windows.go#ne...
src/pkg/os/file_windows.go:242: // console writes len(b) bytes to the console
File.
On 2012/08/28 08:24:40, bsiegert wrote:
> correct function name, s/console/consoleWrite/

Done.

http://codereview.appspot.com/6488044/diff/1001/src/pkg/os/file_windows.go#ne...
src/pkg/os/file_windows.go:244: func (f *File) consoleWrite(b []byte) (n int,
err error) {
On 2012/08/28 08:24:40, bsiegert wrote:
> Maybe I would call this writeConsole.

Done.

[minux1, 2012-08-29 03:46:13]

On Wed, Aug 29, 2012 at 10:33 AM, <alex.brainman@gmail.com> wrote:

> On 2012/08/28 17:04:59, minux wrote:
>
>> I think you also need to do the same for the runtime package
>> (possibly in a follow-up CL)
>>
http://tip.golang.org/src/pkg/**runtime/thread_windows.c#L117<http://tip.gola...
>>
>
> I disagree here. I think print and println can stay as is. These are for
> debugging only. I do not think complications are worth it. But feel free
> to pursue your proposal one way or the other.

runtime.write is used by runtime.printf, which in turn is used by traceback
code. if a program uses a lot of goroutines, i think the same console
overflow
problem could occur during trackback
(http://code.google.com/p/go/issues/detail?id=3376#c2)

[brainman, 2012-08-29 04:44:18]

On 2012/08/29 03:46:13, minux wrote:
> (http://code.google.com/p/go/issues/detail?id=3376#c2)

How can you tell it is Go stack trace?

Alex

[minux1, 2012-08-29 04:56:37]

On Wed, Aug 29, 2012 at 12:44 PM, <alex.brainman@gmail.com> wrote:

> On 2012/08/29 03:46:13, minux wrote:
>
>>
(http://code.google.com/p/go/**issues/detail?id=3376#c2<http://code.google.com...
>> )
>
> How can you tell it is Go stack trace?
>
that comment doesn't mention stack trace, but i assume if we use WriteFile
to write a very long stack trace the same overflow problem could occur.

[brainman, 2012-08-29 05:09:58]

On 2012/08/29 04:56:37, minux wrote:
> >
> that comment doesn't mention stack trace, but i assume if we use WriteFile
> to write a very long stack trace the same overflow problem could occur.

From what I can tell, it is how much of data you supply in one api call (it
tends to break if buffer is ~ 32K). And we do not supply as much during stack
trace, are we? Also, I think WriteConsole will have same problem ...

Alex

[rsc, 2012-09-11 01:18:22]

LGTM

Very happy to see this fixed.

http://codereview.appspot.com/6488044/diff/8001/src/pkg/os/file_windows.go
File src/pkg/os/file_windows.go (right):

http://codereview.appspot.com/6488044/diff/8001/src/pkg/os/file_windows.go#ne...
src/pkg/os/file_windows.go:261: if len(runes) > 0 {
Do we know that WriteConsole will gracefully handle too much data and do a short
write instead of giving an error? If not, maybe

const max = 16000
for len(runes) > 0 {
    m := len(runes)
    if m > maxWrite {
        m = maxWrite
    }
    chunk = runes[:m]
    runes = runes[m:]
    uint16s := utf16.Encode(runes)
    ...
}

Either way, whatever you think is best.

[brainman, 2012-09-12 02:04:58]

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

os: detect and handle console in File.Write on windows

Fixes issue 3376.

R=golang-dev, bsiegert, minux.ma, rsc
CC=golang-dev
http://codereview.appspot.com/6488044

http://codereview.appspot.com/6488044/diff/8001/src/pkg/os/file_windows.go
File src/pkg/os/file_windows.go (right):

http://codereview.appspot.com/6488044/diff/8001/src/pkg/os/file_windows.go#ne...
src/pkg/os/file_windows.go:261: if len(runes) > 0 {
On 2012/09/11 01:18:22, rsc wrote:
> Do we know that WriteConsole will gracefully handle too much data and do a
short
> write instead of giving an error? ...
> 

I do not know yet. I will submit this one as is and deal with big writes in a
separate CL. I might even come up with a test for it.

I am not very keen on fiddling with io buffers "under the covers", but I see no
better way to fix our problem. Having to do it for "console" io only makes it
easier for me. Oh well.