os OpenFile gives Error Invalid Argument for file path strings with padded null bytes

[thiremani]

What version of Go are you using (go version)?

1.10

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

darwin amd64

What did you do?

https://play.golang.org/p/yVd9EYkhbo-

What did you expect to see?

A more informative error which says file path too long.

This was hard to debug too, because printing the string did not show string was of length 255 bytes. (As shown in the link to reproduce the error). I had to print bytes from which the string was created to know the string was too long.

What did you see instead?

Error of type *PathError, which says "Invalid argument"

[peczenyj]

Hello

my 2 cents

The error came from this line

r, e = syscall.Open(name, flag|syscall.O_CLOEXEC, syscallMode(perm))

and it returns an EINVAL ( 0x16 ) error code, this is not related to size but with the arguments. you may need investigate why this happens inside the syscall

syscall.Open call openat( ... ) who performs

	var _p0 *byte
	_p0, err = BytePtrFromString(path)
	if err != nil {
		return
	}

this returns the 0x16 ( see https://play.golang.org/p/zfFyJt2XKQF )

and if you look at the documentation:

https://golang.org/pkg/syscall/#BytePtrFromString

If s contains a NUL byte at any location, it returns (nil, EINVAL).

you may consider this is not clear since the open syscall may return EINVAL in some specific cases too, and this error came from a sanity check before the syscall itself - it is aprecondition error. Can we propose some enhancement?

If I may suggest, I think ByteSliceFromString should not return a pure EINVAL, but maybe a wrapper around EINVAL with message (or Cause) "string contains null byte"

or

we can create an E_PRECOND_FAIL

[thiremani]

I like both suggestions. E_PRECOND_FAIL seems better as ByteSliceFromString does exactly what it promises to do.

On a side note, why does ByteSliceFromString throw error for NUL bytes? A standard C string is NULL terminated, so once we hit the NULL byte, no need to go ahead. Then copy that string into a byte array and return.

[peczenyj]

I can imagine some reasons to prevent send an String with null bytes, like security. Go can be a proxy to a buffer-overflow attack, etc.

[thiremani]

Fair point

[bradfitz]

On a side note, why does ByteSliceFromString throw error for NUL bytes? A standard C string is NULL terminated, so once we hit the NULL byte, no need to go ahead. Then copy that string into a byte array and return.

A C string does, yes, but a Go string does not, and ByteSliceFromString takes a Go string. If you give it a string with NULLs, it's a big signal that you're doing something wrong, and we'll by definition lose data when converting it to a C string, since len(goString with null) != C.strlen(resulting C string). So returning invalid makes sense.

I don't think it's worth inventing new error codes like "E_PRECOND_FAIL" for this. The user will get an error, Fatalf("%q") their input string, see the null bytes, and fix their bug. Considering this is the first bug report in 6 years about this, it probably doesn't happen enough to warrant adding new API surface.

[slrz]

These would be breaking changes. Although the syscall package is not part of the Go1 promise, gratuitous API changes should be avoided, if reasonably possible.

That said, EINVAL seems to be a reasonable choice upon encountering an invalid argument. It's also what programs already know to check for.

[thiremani]

I don't think it's worth inventing new error codes like "E_PRECOND_FAIL" for this. The user will get an error, Fatalf("%q") their input string, see the null bytes, and fix their bug. Considering this is the first bug report in 6 years about this, it probably doesn't happen enough to warrant adding new API surface.

Yes, I agree. Perhaps could be because I'm new to go, it took me a while to debug. What foxed me was fmt.Println(string) only printed the string without the padded 0 bytes. I see now that fmt.Printf("%q", string) actually shows the zero bytes.

Maybe changing the fmt.Println(string) function is an option here? The fact that Println could hide from the user certain aspects of the string is worrying. It gives a misleading impression of the length of the string. Though len(string) clears up that, a general user depends heavily upon the Print statement for his day-to-day debugging/information gathering.

Also appending to such a string gives us a similar issue as highlighted here:
https://play.golang.org/p/pHK8mFuGuao

The NUL bytes are completely ignored by Println!

[bradfitz]

Sorry, we can't change fmt.Print.

Doing,

	fmt.Print("foo\x00\x00")

Has to literally print the input.

If you redirect it to a file, you should get exactly those bytes. Changing fmt would break people's programs and corrupt data.

Changing the log package might be possible, though. You can file a proposal for that if you'd like.

Hope you're enjoying Go besides this. :)

[thiremani]

If you redirect it to a file, you should get exactly those bytes. Changing fmt would break people's programs and corrupt data.

Tested this and you're perfectly right. Okay, so its only when Println goes to stdout it does not show the NUL bytes in the string. Not as important.

I actually quite like Go. It's amazingly quick to develop and test.

[cznic]

Okay, so its only when Println goes to stdout it does not show the NUL bytes in the string.

That's conflating data and their presentation. The zero/NUL bytes are written to whatever is connected to stdout and can be seen/revealed when needed. It's just the terminal that may chose not to render them visible - but that's also true for other code points as well.

[thiremani]

My bad. Fully agree.