code review 6458050: syscall: return EINVAL when string arguments have NUL c...

syscall: return EINVAL when string arguments have NUL characters

Since NUL usually terminates strings in underlying syscalls, allowing
it when converting string arguments is a security risk, especially
when dealing with filenames. For example, a program might reason that
filename like "/root/..\x00/" is a subdirectory or "/root/" and allow
access to it, while underlying syscall will treat "\x00" as an end of
that string and the actual filename will be "/root/..", which might
be unexpected. Returning EINVAL when string arguments have NUL in
them makes sure this attack vector is unusable.

[snaury, 2012-07-30 20:44:14]

Hello golang-dev@googlegroups.com,

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

[snaury, 2012-07-30 20:47:04]

On Tue, Jul 31, 2012 at 12:44 AM,  <snaury@gmail.com> wrote:
> syscall: return EINVAL when string arguments have NUL characters
>
> Since NUL usually terminates strings in underlying syscalls, allowing
> it when converting string arguments is a security risk, especially
> when dealing with filenames. For example, a program might reason that
> filename like "/root/..\x00/" is a subdirectory or "/root/" and allow
> access to it, while underlying syscall will treat "\x00" as an end of
> that string and the actual filename will be "/root/..", which might
> be unexpected. Returning EINVAL when string arguments have NUL in
> them makes sure this attack vector is unusable.
>
> Please review this at http://codereview.appspot.com/6458050/

Please note however that I only ran the test suite on darwin. I
haven't tested any other OS beyond running GOOS=platform GOARCH=386
./make.bash to make sure it builds, so it probably needs a lot of
testing to make sure it doesn't break any platform in unexpected ways.

[r, 2012-07-30 23:25:54]

This is a pretty big change and I'm not convinced you need to work this hard. I
see two simpler approaches:

1) Just terminate the string at any NUL byte.
2) Panic if you see a NUL byte.

And then there's this CL:

3) Generate an error if there's a NUL byte and update all places that handle
strings from the user.

Essay question: compare and contrast.

[bradfitz, 2012-07-30 23:36:42]

On Mon, Jul 30, 2012 at 4:25 PM, <r@golang.org> wrote:

> This is a pretty big change and I'm not convinced you need to work this
> hard. I see two simpler approaches:
>
> 1) Just terminate the string at any NUL byte.
>

Go is not C.  That feels wrong.


> 2) Panic if you see a NUL byte.
>

That also feels wrong.  I don't want input from users (GET /foo%00
HTTP/1.1) to panic my programs any more than os.Open("/file-not-exist")
panicing my programs.

And then there's this CL:
>
> 3) Generate an error if there's a NUL byte


Go style is to use errors, no?


> and update all places that
> handle strings from the user.
>

Isn't this what Russ asked for?

[r, 2012-07-30 23:46:13]

On Mon, Jul 30, 2012 at 4:36 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> On Mon, Jul 30, 2012 at 4:25 PM, <r@golang.org> wrote:
>>
>> This is a pretty big change and I'm not convinced you need to work this
>> hard. I see two simpler approaches:
>>
>> 1) Just terminate the string at any NUL byte.
>
>
> Go is not C.  That feels wrong.

But the Unix syscall interface isn't using Go strings either. I'm not
saying this is the right answer, but I am dismayed by the amount of
extra processing and handling required to let the user know something
that's probably not even interesting.

>> 2) Panic if you see a NUL byte.
>
>
> That also feels wrong.  I don't want input from users (GET /foo%00 HTTP/1.1)
> to panic my programs any more than os.Open("/file-not-exist") panicing my
> programs.

See Point 1.

>> And then there's this CL:
>>
>> 3) Generate an error if there's a NUL byte
>
>
> Go style is to use errors, no?

Yes, but this one... well, it just bugs me. Goo is creeping up through the
code.

> Isn't this what Russ asked for?

That doesn't mean I can't question it.

Again, I'm asking for discussion here. This is a huge change when
compared to the likelihood of trouble. Is the tradeoff right?

-rob

[r, 2012-07-30 23:54:27]

http://codereview.appspot.com/6458050/diff/4001/src/pkg/syscall/syscall.go
File src/pkg/syscall/syscall.go (right):

http://codereview.appspot.com/6458050/diff/4001/src/pkg/syscall/syscall.go#ne...
src/pkg/syscall/syscall.go:17: // StringByteSlice returns a NUL-terminated slice
of bytes
assuming we're going with the design proposed in this CL, this function is now
deprecated and, in fact, all but unused. it should be marked deprecated and the
new function should be exported.

http://codereview.appspot.com/6458050/diff/4001/src/pkg/syscall/syscall.go#ne...
src/pkg/syscall/syscall.go:29: for i := 0; i < len(s); i++ {
for i, c := range s

but this is doing a lot of copying and other stuff that is rarely necessary.
make the common path fast.
faster but untested:

var i int
var c rune
for i, c = range s {
  if c == '\x00' {
    break
  }
}
a = make([]byte, i+1)
copy(a, s)
return a

[r, 2012-07-31 00:17:16]

Gri proposes option 4:

4) Since the returned []byte is different from the string anyway (it
is one byte longer), if we find an unexpected NUL we can return a name
that signals that:

  foo\x00bar becomes, say foo%NULBYTE%bar

That means no API change is necessary, error messages will look good,
but leaves one ugly corner case I can think of, which is creating a
file with such an unpleasant name.

-rob

[r, 2012-07-31 01:05:09]

Okay I have convinced myself we need to do the error version. let's make it
as palatable as possible

On Monday, July 30, 2012, Rob Pike wrote:

> Gri proposes option 4:
>
> 4) Since the returned []byte is different from the string anyway (it
> is one byte longer), if we find an unexpected NUL we can return a name
> that signals that:
>
>   foo\x00bar becomes, say foo%NULBYTE%bar
>
> That means no API change is necessary, error messages will look good,
> but leaves one ugly corner case I can think of, which is creating a
> file with such an unpleasant name.
>
> -rob
>

[snaury, 2012-07-31 05:30:42]

On Tue, Jul 31, 2012 at 3:54 AM,  <r@golang.org> wrote:
>
http://codereview.appspot.com/6458050/diff/4001/src/pkg/syscall/syscall.go#ne...
> src/pkg/syscall/syscall.go:17: // StringByteSlice returns a
> NUL-terminated slice of bytes
> assuming we're going with the design proposed in this CL, this function
> is now deprecated and, in fact, all but unused. it should be marked
> deprecated and the new function should be exported.

Initially I made it that way for several reasons:

- I wasn't sure all relevant syscalls had an error result, but
searching now it turns out there are indeed no calls to the old
function, so you may be right
- Since it's exported it's likely intended for use in other packages,
it's not our job to force all users to use new API, checking for
conversion errors in user code (especially in tests) is clumsy and
often not needed, and is only relatively easy to do in syscall package
because it's mostly auto-generated.

There are 5 new "safe" functions:

- safeStringByteSlice
- safeStringBytePtr
- safeStringSlicePtr
- safeStringToUTF16
- safeStringToUTF16Ptr

Should all be exported? Not all old functions can be deprecated though.

>
http://codereview.appspot.com/6458050/diff/4001/src/pkg/syscall/syscall.go#ne...
> src/pkg/syscall/syscall.go:29: for i := 0; i < len(s); i++ {
> for i, c := range s
>
> but this is doing a lot of copying and other stuff that is rarely
> necessary.
> make the common path fast.
> faster but untested:
>
> var i int
> var c rune
> for i, c = range s {
>   if c == '\x00' {
>     break
>   }
> }
> a = make([]byte, i+1)
> copy(a, s)
> return a

Are you sure it's faster? I didn't use range to avoid useless UTF-8
decoding, I didn't use copy in the end because then we would scan the
input string twice (though there's an extra index check on write, so
maybe copy would indeed be faster on the common case of short
strings). I'll try to make a benchmark in the evening (that's in ~12
hours, moscow time) and see it for real.

P.S. I'll update a changeset now, but it only changes a single place
where unsafe conversion was used in Getenv on Windows, it doesn't
change anything else. I'll only be able to see to other changes in the
evening.

[snaury, 2012-07-31 05:31:44]

Hello golang-dev@googlegroups.com, r@golang.org, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[snaury, 2012-07-31 05:37:51]

On Tue, Jul 31, 2012 at 9:30 AM, Alexey Borzenkov <snaury@gmail.com> wrote:
> There are 5 new "safe" functions:
>
> - safeStringByteSlice
> - safeStringBytePtr
> - safeStringSlicePtr
> - safeStringToUTF16
> - safeStringToUTF16Ptr
>
> Should all be exported? Not all old functions can be deprecated though.

On the other hand, maybe they should be. I just realized at least
Windows versions are used in other packages (e.g.
pkg/os/file_windows.go, func openDir) that need to use safe variants.
So, do I export these new safe functions?

[r, 2012-07-31 05:45:40]

You're right about avoiding range in the loop but I had a larger
point: don't copy the bytes one a time if you don't have to, and you
almost never have to.  So it's more like this:

var i int
for i := 0; i < len(s); i++ {
  if s[i] == '\x00' {
    return nil, syscall.EINVAL
  }
}
a = make([]byte, i+1)
copy(a, s)
return a

[r, 2012-07-31 05:46:57]

The versions of these functions that obsolete existing exported
functions should be exported.  I don't like the 'safe' name much
though. I'll sleep on it.

-rob

[albert.strasheim, 2012-07-31 06:16:26]

Hello

Since the syscall package isn't exhaustive, there's probably quite a few 
syscall wrapper functions in the world that might need everything that is 
available to the wrappers inside the syscall package.

Regards

Albert

On Tuesday, July 31, 2012 7:46:57 AM UTC+2, Rob Pike wrote:
>
> The versions of these functions that obsolete existing exported 
> functions should be exported.  I don't like the 'safe' name much 
> though. I'll sleep on it. 
>
> -rob
>

[snaury, 2012-07-31 19:05:26]

Hello golang-dev@googlegroups.com, r@golang.org, bradfitz@golang.org,
fullung@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[r, 2012-07-31 19:49:56]

http://codereview.appspot.com/6458050/diff/4004/src/pkg/syscall/syscall.go
File src/pkg/syscall/syscall.go (right):

http://codereview.appspot.com/6458050/diff/4004/src/pkg/syscall/syscall.go#ne...
src/pkg/syscall/syscall.go:27: func SafeStringByteSlice(s string) (a []byte, err
error) {
This is looking better. I don't like the "Safe" name though because it's about
the bug this version fixes, not what the function actually does. It carries no
information. What does "safe" mean? And whose safety is it preserving? The doc
comment should describe the safety property; the name should be what it does.

There are two ways to go.

1) Be really clear: StringByteSliceNULTerminated or some such.
2) Just pick a different name that means the same thing, and let the doc comment
carry the weight: StringToByteSlice. The name doesn't really say everything, but
don't forget that outsiders will see it as syscall.StringToByteSlice, which is
pretty evocative.

I vote for option 2, giving us:

// StringByteSlice returns a NUL-terminated slice of bytes
// containing the text of s unmodified. It does not check for
// NUL bytes within the string, so it should be used with care.
// StringToByteSlice is almost always a better choice when
// passing strings to system calls.
func StringByteSlice(s string) []byte {
...

// StringToByteSlice returns a NUL-terminated slice of bytes containing
// the text of s. If s contains a NUL byte at any location, it returns
// EINVAL. This is the correct routine to use when passing strings
// to system calls.
func StringToByteSlice(s string) (a []byte, err error) {

[rsc, 2012-07-31 19:53:06]

StringToByteSlice SGTM.

I think StringByteSlice could check for NUL and panic (be a thin wrapper around
StringToByteSlice).

[snaury, 2012-07-31 20:10:14]

On Tue, Jul 31, 2012 at 11:53 PM,  <rsc@golang.org> wrote:
> StringToByteSlice SGTM.
>
> I think StringByteSlice could check for NUL and panic (be a thin wrapper
> around StringToByteSlice).

It seems you are forgetting that there are Windows variants of these
APIs as well, which are already named String*To*UTF16, etc. (which are
actually widely used outside syscall, far more so than StringByteSlice
and StringBytePtr), besides there would be very little distinction
between function names, almost like a typo, very hard to spot right
away.

What about a suffix of *NoNUL, like StringByteSliceNoNUL,
StringBytePtrNoNUL, etc.?

Panicking is a bad idea in my opinion. This CL should be about fixing
potential vulnerabilities, not introducing new ones (think denial of
service) in pre-existing code.

[snaury, 2012-07-31 20:16:45]

On Tue, Jul 31, 2012 at 11:49 PM,  <r@golang.org> wrote:
>
> http://codereview.appspot.com/6458050/diff/4004/src/pkg/syscall/syscall.go
> File src/pkg/syscall/syscall.go (right):
>
>
http://codereview.appspot.com/6458050/diff/4004/src/pkg/syscall/syscall.go#ne...
> src/pkg/syscall/syscall.go:27: func SafeStringByteSlice(s string) (a
> []byte, err error) {
> This is looking better. I don't like the "Safe" name though because it's
> about the bug this version fixes, not what the function actually does.

Of course, you said you'll sleep on it, so this was a temporary update
(it was primarily about exporting and fixing more packages that use
syscall conversion functions). See my comment to Russ about a possible
different name. Right now "safe" prefix is playing a very good role in
my tree by just being an easy to search marker (search for \bSafe to
see all uses of new functions, search for \bString for all uses of old
functions), thus being easy to replace to whatever we choose.

[r, 2012-07-31 21:06:38]

The Windows naming problem is easily resolved by reversing the names
of the new functions, so: ByteSliceFromString (and BytePtrFromString)
and UTF16FromString. A name like StringByteSliceNoNUL is poor because
it contradicts its purpose: it *adds* a NUL.

Regarding panicking, the Go 1  compatibility document  allows this:

"Security. A security issue in the specification or implementation may
come to light whose resolution requires breaking compatibility. We
reserve the right to address such security issues."

I argue that this is a security hole that should be closed. We will
also eliminate all uses of the old functions from the standard tree
and deprecate those functions.

-rob

[snaury, 2012-07-31 21:28:05]

On Wed, Aug 1, 2012 at 1:06 AM, Rob Pike <r@golang.org> wrote:
> The Windows naming problem is easily resolved by reversing the names
> of the new functions, so: ByteSliceFromString (and BytePtrFromString)
> and UTF16FromString. A name like StringByteSliceNoNUL is poor because
> it contradicts its purpose: it *adds* a NUL.

Ok, I think I like this too, though it will need a little more work.

> Regarding panicking, the Go 1  compatibility document  allows this:
[...]
> I argue that this is a security hole that should be closed. We will
> also eliminate all uses of the old functions from the standard tree
> and deprecate those functions.

The problem that I see with panicking is that it doesn't resolve a
security issue, it actually *adds* a new one. However we could limit
the scope of the panic by not using old functions in the standard
library at all (and thus deprecating them all). I can see one way to
do it: make error an advisement, e.g. if NUL byte is detected always
do the conversion anyway, *in addition to*, returning an error. This
way test code (as well as code that doesn't care about NUL bytes or
actually needs embedded NULs for some reason) would be able to use
BytePtrFromString(...)[0], etc. What do you think?

[r, 2012-07-31 22:46:34]

Let's leave the question of panic aside for this CL, which is big
enough already. Let's just get the new functions designed and
installed.

In any case we would not be using the old functions in the standard
library, and this CL should delete all references to them.

Let me think about the question regarding what to do on error.

-rob

[snaury, 2012-08-01 06:12:41]

Hello golang-dev@googlegroups.com, r@golang.org, bradfitz@golang.org,
fullung@gmail.com, rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[snaury, 2012-08-01 18:56:30]

Hello golang-dev@googlegroups.com, r@golang.org, bradfitz@golang.org,
fullung@gmail.com, rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[snaury, 2012-08-01 19:35:59]

Now fixed a small typo in a windows test and successfully tested on windows/386.
Also all tests pass on linux/amd64.

[minux1, 2012-08-01 19:47:44]

On 2012/08/01 19:35:59, snaury wrote:
> Now fixed a small typo in a windows test and successfully tested on
windows/386.
> Also all tests pass on linux/amd64.
all test passed on darwin/amd64 and darwin/386.

[r, 2012-08-01 20:24:11]

http://codereview.appspot.com/6458050/diff/14003/src/pkg/syscall/syscall.go
File src/pkg/syscall/syscall.go (right):

http://codereview.appspot.com/6458050/diff/14003/src/pkg/syscall/syscall.go#n...
src/pkg/syscall/syscall.go:18: // containing the text of s.
Please add here the commentary I wrote in a prior review that tells people to
use the new functions. Same for StringBytePtr.

http://codereview.appspot.com/6458050/diff/14003/src/pkg/syscall/syscall.go#n...
src/pkg/syscall/syscall.go:27: // location it additionally returns EINVAL.
I think on error it should return
nil, EINVAL
People shouldn't be ignoring this error; it's a security risk.

[snaury, 2012-08-01 20:46:45]

Hello golang-dev@googlegroups.com, r@golang.org, bradfitz@golang.org,
fullung@gmail.com, rsc@golang.org, minux.ma@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[r, 2012-08-01 21:08:40]

http://codereview.appspot.com/6458050/diff/3034/src/pkg/crypto/x509/root_wind...
File src/pkg/crypto/x509/root_windows.go (right):

http://codereview.appspot.com/6458050/diff/3034/src/pkg/crypto/x509/root_wind...
src/pkg/crypto/x509/root_windows.go:102: if e != nil {
just use err here (it's declared below already).
in many of these functions, e is used when err would be better, but don't change
them. here err is the local standard (and in general the preferred style) so use
it.

http://codereview.appspot.com/6458050/diff/3034/src/pkg/os/error_test.go
File src/pkg/os/error_test.go (right):

http://codereview.appspot.com/6458050/diff/3034/src/pkg/os/error_test.go#newc...
src/pkg/os/error_test.go:88: return
delete. Fatal returns.
same below

http://codereview.appspot.com/6458050/diff/3034/src/pkg/os/error_test.go#newc...
src/pkg/os/error_test.go:99: t.Fatalf("open ErrPathNUL second: %s", err)
s/ second//

http://codereview.appspot.com/6458050/diff/3034/src/pkg/syscall/env_windows.go
File src/pkg/syscall/env_windows.go (right):

http://codereview.appspot.com/6458050/diff/3034/src/pkg/syscall/env_windows.g...
src/pkg/syscall/env_windows.go:37: func Setenv(key, value string) (err error) {
there's no reason to declare err here

http://codereview.appspot.com/6458050/diff/3034/src/pkg/syscall/exec_plan9.go
File src/pkg/syscall/exec_plan9.go (right):

http://codereview.appspot.com/6458050/diff/3034/src/pkg/syscall/exec_plan9.go...
src/pkg/syscall/exec_plan9.go:68: // arrays of strings to system calls.
while we're here let's make this a more canonical doc comment

StringSlicePtr converts a slice of strings to a slice of pointers to
NUL-terminated byte slices.
This function is a security risk because it does not check for the
presence of NUL bytes in the strings.  SlicePtrFromStrings should
be used instead when the result is to be passed to a system call;
StringSlicePtr remains only for backward compatibility.

http://codereview.appspot.com/6458050/diff/3034/src/pkg/syscall/exec_plan9.go...
src/pkg/syscall/exec_plan9.go:82: // passing arrays of strings to system calls.
ditto:


SlicePtrFromStrings converts a slice of strings to a slice of pointers to
NUL-terminated byte slices. If any of the strings contain NUL bytes,
it returns (nil, EINVAL).

http://codereview.appspot.com/6458050/diff/3034/src/pkg/syscall/exec_unix.go
File src/pkg/syscall/exec_unix.go (right):

http://codereview.appspot.com/6458050/diff/3034/src/pkg/syscall/exec_unix.go#...
src/pkg/syscall/exec_unix.go:72: func StringSlicePtr(ss []string) []*byte {
copy rewritten comments from other file.

http://codereview.appspot.com/6458050/diff/3034/src/pkg/syscall/syscall.go
File src/pkg/syscall/syscall.go (right):

http://codereview.appspot.com/6458050/diff/3034/src/pkg/syscall/syscall.go#ne...
src/pkg/syscall/syscall.go:21: // passing strings to system calls.
copy comments from other file.

http://codereview.appspot.com/6458050/diff/3034/src/pkg/syscall/syscall_windo...
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/6458050/diff/3034/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:60: func StringToUTF16(s string) []uint16 {
return utf16.Encode([]rune(s + "\x00")) }
copy (and adapt) comments from other file.

[snaury, 2012-08-01 21:43:40]

Hello golang-dev@googlegroups.com, r@golang.org, bradfitz@golang.org,
fullung@gmail.com, rsc@golang.org, minux.ma@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[rsc, 2012-08-01 21:49:43]

approach looks good. i didn't read all the files.

http://codereview.appspot.com/6458050/diff/14044/src/pkg/syscall/syscall.go
File src/pkg/syscall/syscall.go (right):

http://codereview.appspot.com/6458050/diff/14044/src/pkg/syscall/syscall.go#n...
src/pkg/syscall/syscall.go:19: // This function is a security risk because it
does not check for
This should panic. On the review someone said that panicking is itself a
security risk. I disagree but even so it is a smaller one.

a, err := ByteSliceFromString(s)
if err != nil {
    panic("syscall: string with NUL passed to StringByteSlice")
}
return a

Then the comment can be significantly simpler:

// StringByteSlice is deprecated. Use ByteSliceFromString instead.
// StringByteSlice is like ByteSliceFromString, but if s contains a NUL,
// StringByteSlice panics instead of returning an error.

[snaury, 2012-08-01 21:54:01]

On Thu, Aug 2, 2012 at 1:49 AM,  <rsc@golang.org> wrote:
> This should panic. On the review someone said that panicking is itself a
> security risk. I disagree but even so it is a smaller one.
>
> a, err := ByteSliceFromString(s)
> if err != nil {
>     panic("syscall: string with NUL passed to StringByteSlice")
> }
> return a
>
> Then the comment can be significantly simpler:

I think Rob said above that he will deal with panicking in a separate CL.

[snaury, 2012-08-05 18:09:44]

On 2012/08/01 21:49:43, rsc wrote:
> This should panic. On the review someone said that panicking is itself a
> security risk. I disagree but even so it is a smaller one.

Ok, since it seems panics are going to happen anyway, I'm changing my patch to
add panicking and fix comments.

[snaury, 2012-08-05 18:11:17]

Hello golang-dev@googlegroups.com, r@golang.org, bradfitz@golang.org,
fullung@gmail.com, rsc@golang.org, minux.ma@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[snaury, 2012-08-05 18:18:45]

Hello golang-dev@googlegroups.com, r@golang.org, bradfitz@golang.org,
fullung@gmail.com, rsc@golang.org, minux.ma@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[rsc, 2012-08-05 21:18:45]

LGTM

[rsc, 2012-08-05 21:24:37]

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

syscall: return EINVAL when string arguments have NUL characters

Since NUL usually terminates strings in underlying syscalls, allowing
it when converting string arguments is a security risk, especially
when dealing with filenames. For example, a program might reason that
filename like "/root/..\x00/" is a subdirectory or "/root/" and allow
access to it, while underlying syscall will treat "\x00" as an end of
that string and the actual filename will be "/root/..", which might
be unexpected. Returning EINVAL when string arguments have NUL in
them makes sure this attack vector is unusable.

R=golang-dev, r, bradfitz, fullung, rsc, minux.ma
CC=golang-dev
http://codereview.appspot.com/6458050

Committer: Russ Cox <rsc@golang.org>

[yk, 2012-08-06 10:08:50]

the commit breaks builds on Plan 9: 

go build runtime: fork/exec /go/pkg/tool/plan9_386/8g: bad arg in system call

[snaury, 2012-08-06 11:58:02]

I think I understand why. On plan9 path separator is NUL, so in env
embedded NULs is a feature, so should be converted differently.
On Aug 6, 2012 2:08 PM, <yarikos@gmail.com> wrote:

> the commit breaks builds on Plan 9:
>
> go build runtime: fork/exec /go/pkg/tool/plan9_386/8g: bad arg in system
> call
>
>
>
http://codereview.appspot.com/**6458050/<http://codereview.appspot.com/6458050/>
>

[yk, 2012-08-06 13:12:32]

The path separator is the same as on unix here.
Here's what go -v -x says:

term% /go/pkg/tool/plan9_386/go_bootstrap build -v -x runtime
WORK=/tmp/go-build103475222
runtime
mkdir -p $WORK/runtime/_obj/
cd /go/src/pkg/runtime
/go/pkg/tool/plan9_386/8g -o $WORK/runtime/_obj/_go_.8 -p runtime -+ -D
_/go/src/pkg/runtime -I $WORK ./compiler.go ./debug.go ./error.go ./extern.go
./mem.go ./softfloat64.go ./type.go ./zgoarch_386.go ./zgoos_plan9.go
./zruntime_defs_plan9_386.go ./zversion.go
go build runtime: fork/exec /go/pkg/tool/plan9_386/8g: bad arg in system call


What follows is an excerpt from syscall trace near the point of failure:

149868 go_bootstrap Pwrite e6bfa 1 
0x10755000/"cd./go/src/pkg/runtime./go/pkg/tool/plan9_386/8g.-o.$WORK/runtim"
286 -1cd /go/src/pkg/runtime
/go/pkg/tool/plan9_386/8g -o $WORK/runtime/_obj/_go_.8 -p runtime -+ -D
_/go/src/pkg/runtime -I $WORK ./compiler.go ./debug.go ./error.go ./extern.go
./mem.go ./softfloat64.go ./type.go ./zgoarch_386.go ./zgoos_plan9.go
./zruntime_defs_plan9_386.go ./zversion.go
 = 286 "" 1344258448894496659 1344258448894766522
149868 go_bootstrap Stat e6ba7 0x1073efe0/"/go/pkg/tool/plan9_386/8g" 0x106fd180
113 = 63 "" 1344258448895335579 1344258448896411677
149868 go_bootstrap Open e6ba7 0x10743890/"/dev/null" 0x0 = 4 ""
1344258448897079628 1344258448897384690
149868 go_bootstrap Pipe e6ba7 0x10715200 = 0 "" 1344258448898021633
1344258448898080298
149868 go_bootstrap Close e6ba7 4 = 0 "" 1344258448898846305 1344258448898855524
149868 go_bootstrap Close e6ba7 6 = 0 "" 1344258448899517609 1344258448899527666
149868 go_bootstrap Close e6ba7 5 = 0 "" 1344258448900188913 1344258448900201484
149868 go_bootstrap Pread 45737 3 0x30711e28 8 0 0x30711e28/"......&F" 8 0 = 8
"" 1344258448900819989 1344258448900831722
149868 go_bootstrap Pwrite e6bfa 2 
0x10717140/"go.build.runtime:.fork/exec./go/pkg/tool/plan9_386/8g:.bad.arg.i" 78
-1go build runtime: fork/exec /go/pkg/tool/plan9_386/8g: bad arg in system call

Thanks.

[snaury, 2012-08-06 17:01:56]

On 2012/08/06 13:12:32, yarikos wrote:
> The path separator is the same as on unix here.
> Here's what go -v -x says:

Could you please see if http://codereview.appspot.com/6454104 fixes plan9 build?

[yk, 2012-08-07 07:40:49]

> Could you please see if http://codereview.appspot.com/6454104 fixes plan9
build?

yes, it does. 
Спасибо!