Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

plan9 builds broken on go1.18-b69b2f63d6 with garble @ 64cbbbaa #417

Closed
kortschak opened this issue Nov 13, 2021 · 6 comments · Fixed by #428
Closed

plan9 builds broken on go1.18-b69b2f63d6 with garble @ 64cbbbaa #417

kortschak opened this issue Nov 13, 2021 · 6 comments · Fixed by #428

Comments

@kortschak
Copy link
Contributor

What version of Garble and Go are you using?

$ git rev-parse HEAD
64cbbbaa0feaa2cbc5dda77bb85f7e1c89851e25

$ go version
go version devel go1.18-b69b2f63d6 Fri Nov 12 23:35:31 2021 +0000 linux/amd64

What environment are you running Garble on?

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN="/home/user/bin"
GOCACHE="/home/user/.cache/go-build"
GOENV="/home/user/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/user/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/user"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/user/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/user/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="devel go1.18-b69b2f63d6 Fri Nov 12 23:35:31 2021 +0000"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/user/src/github.com/kortschak/toutoumomoma/go.mod"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build1358491072=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Run go test in github.com/kortschak/toutoumomoma. This involves cross-compiled builds including for plan9, which fails. Minimised reproducer not requiring running this test suite listed below.

What did you expect to see?

Successful build.

What did you see instead?

# syscall
/tmp/garble-shared3781820942/syscall/exec_plan9.go:127:5: misplaced compiler directive
exit status 2
exit status 2
# os
/tmp/garble-shared2221653282/os/file_plan9.go:149:5: fullData declared but not used
/tmp/garble-shared2221653282/os/file_plan9.go:151:12: invalid operation: cannot call non-function append (variable of type bool)
exit status 2
exit status 2

exec_plan9.go: //go:norace is placed above empty text having been moved from the correct position above the forkAndExecInChild function definition below dupdev. Reproduced by GOOS=plan9 garble -literals build -o ./testdata/executable ./testdata in github.com/kortschak/toutoumomoma.

// name of the directory containing names and control files for all open file descriptors
var dupdev, _ = BytePtrFromString(func() string {
	data := []byte("\xccd")
	positions := [...]byte{0, 0}
	for i := 0; i < 2; i += 2 {
		localKey := byte(i) + byte(positions[i]^positions[i+1]) + 87
		data[positions[i]], data[positions[i+1]] = data[positions[i+1]]+localKey, data[positions[i]]+localKey

		// forkAndExecInChild forks the process, calling dup onto 0..len(fd)
		// and finally invoking exec(argv0, argvv, envv) in the child.
		// If a dup or exec fails, it writes the error string to pipe.
		// (The pipe write end is close-on-exec so if exec succeeds, it will be closed.)
		//
		// In the child, this function must not acquire any locks, because
		// they might have been locked at the time of the fork. This means
		// no rescheduling, no malloc calls, and no new stack segments.
		// The calls to RawSyscall are okay because they are assembly
		// functions that do not grow the stack.
		//go:norace
	}
	return string(data)
}())

func forkAndExecInChild(argv0 *byte, argv []*byte, envv []envItem, dir *byte, attr *ProcAttr, pipe int, rflag int) (pid int, err error) {
	// Declare all variables at top in case any
	// declarations require heap allocation (e.g., errbuf).

file_plan9.go: append is a bool here, and so shadows the append built-in and breaks string obfuscation. Reproduced by GOOS=plan9 garble -literals -tiny build -o ./testdata/executable ./testdata in github.com/kortschak/toutoumomoma.

// openFileNolog is the Plan 9 implementation of OpenFile.
func openFileNolog(name string, flag int, perm FileMode) (*File, error) {
	var (
		fd	int
		e	error
		create	bool
		excl	bool
		trunc	bool
		append	bool
	)

	if flag&O_CREATE == O_CREATE {
		flag = flag & ^O_CREATE
		create = true
	}
	if flag&O_EXCL == O_EXCL {
		excl = true
	}
	if flag&O_TRUNC == O_TRUNC {
		trunc = true
	}
	// O_APPEND is emulated on Plan 9
	if flag&O_APPEND == O_APPEND {
		flag = flag &^ O_APPEND
		append = true
	}

	if (create && trunc) || excl {
		fd, e = syscall.Create(name, flag, syscallMode(perm))
	} else {
		fd, e = syscall.Open(name, flag)
		if IsNotExist(e) && create {
			fd, e = syscall.Create(name, flag, syscallMode(perm))
			if e != nil {
				return nil, &PathError{Op: func() string {
					key := []byte("*x\xad9\x1e\xcd")
					data := []byte("I\n\xc8Xj\xa8")
					for i, b := range key {
						data[i] = data[i] ^ b
					}
					return string(data)
				}(), Path: name, Err: e}
			}
		}
	}

	if e != nil {
		return nil, &PathError{Op: func() string {
			data := []byte("\xb5\x91e\x91")
			positions := [...]byte{3, 0, 1, 3}
			for i := 0; i < 4; i += 2 {
				localKey := byte(i) + byte(positions[i]^positions[i+1]) + 31
				data[positions[i]], data[positions[i+1]] = data[positions[i+1]]-localKey, data[positions[i]]-localKey
			}
			return string(data)
		}(), Path: name, Err: e}
	}

	if append {
		if _, e = syscall.Seek(fd, 0, io.SeekEnd); e != nil {
			return nil, &PathError{Op: func() string {
				fullData := []byte("\x05\x9a \xaf\x856E\xb6")
				var data []byte
				data = append(data, fullData[5]^fullData[6], fullData[4]-fullData[2], fullData[3]+fullData[7], fullData[0]-fullData[1])
				return string(data)
			}(), Path: name, Err: e}
		}
	}

	return NewFile(uintptr(fd), name), nil
}
@mvdan
Copy link
Member

mvdan commented Nov 13, 2021

Thanks for reporting. We should probably have an integration test that ensures cross-builds to platforms we're not testing on still work.

@kortschak
Copy link
Contributor Author

#418 prevents the misplaced compiler directive failure in the first case, now falling through to the shadowing of the append built-in by the append bool local var.

@mvdan
Copy link
Member

mvdan commented Nov 14, 2021

I think these plan9 failures are not fully deterministic; I've seen different errors with different runs. Probably given that package compiles happen concurrently.

In any case, I'm aware #418 won't fix this issue. The append build failure is a bug with literal obfuscation, for example.

mvdan added a commit to mvdan/garble-fork that referenced this issue Nov 16, 2021
In the added test case, "garble -literals build" would fail:

	--- FAIL: TestScripts/literals (8.29s)
		testscript.go:397:
			> env GOPRIVATE=test/main
			> garble -literals build
			[stderr]
			# test/main
			Usz1FmFm.go:1: cannot call non-function string (type int), declared at Usz1FmFm.go:1
			Usz1FmFm.go:1: string is not a type
			Usz1FmFm.go:1: cannot call non-function append (type int), declared at Usz1FmFm.go:1

That is, for input code such as:

	var append int
	println("foo")
	_ = append

We'd end up with obfuscated code like:

	var append int
	println(func() string {
		// obfuscation...
		x = append(x, ...)
		// obfuscation...
		return string(x)
	})
	_ = append

Which would then break, as the code is shadowing the "append" builtin.
To work around this, always obfuscate variable names, so we end up with:

	var mwu1xuNz int
	println(func() string {
		// obfuscation...
		x = append(x, ...)
		// obfuscation...
		return string(x)
	})
	_ = mwu1xuNz

This change shouldn't make the quality of our obfuscation stronger,
as local variable names do not currently end up in Go binaries.
However, this does make garble more consistent in treating identifiers,
and it completely avoids any issues related to shadowing builtins.

Moreover, this also paves the way for publishing obfuscated source code,
such as burrowers#369.

Fixes burrowers#417.
@mvdan
Copy link
Member

mvdan commented Nov 16, 2021

#420 fixes the immediate append shadowing bug with -literals. I've left a comment in #240 (comment) to remind ourselves to have more integration testing with other GOOS/GOARCH pairs like plan9.

mvdan added a commit to mvdan/garble-fork that referenced this issue Nov 16, 2021
In the added test case, "garble -literals build" would fail:

	--- FAIL: TestScripts/literals (8.29s)
		testscript.go:397:
			> env GOPRIVATE=test/main
			> garble -literals build
			[stderr]
			# test/main
			Usz1FmFm.go:1: cannot call non-function string (type int), declared at Usz1FmFm.go:1
			Usz1FmFm.go:1: string is not a type
			Usz1FmFm.go:1: cannot call non-function append (type int), declared at Usz1FmFm.go:1

That is, for input code such as:

	var append int
	println("foo")
	_ = append

We'd end up with obfuscated code like:

	var append int
	println(func() string {
		// obfuscation...
		x = append(x, ...)
		// obfuscation...
		return string(x)
	})
	_ = append

Which would then break, as the code is shadowing the "append" builtin.
To work around this, always obfuscate variable names, so we end up with:

	var mwu1xuNz int
	println(func() string {
		// obfuscation...
		x = append(x, ...)
		// obfuscation...
		return string(x)
	})
	_ = mwu1xuNz

This change shouldn't make the quality of our obfuscation stronger,
as local variable names do not currently end up in Go binaries.
However, this does make garble more consistent in treating identifiers,
and it completely avoids any issues related to shadowing builtins.

Moreover, this also paves the way for publishing obfuscated source code,
such as burrowers#369.

Fixes burrowers#417.
@lu4p lu4p closed this as completed in ea2e0bd Nov 16, 2021
@kortschak
Copy link
Contributor Author

I'm still seeing the append shadowing in my test (using go1.18-b69b2f63d6 and ea2e0bd).

$ cd $(go env GOPATH)/src/github.com/kortschak/toutoumomoma
$ git rev-parse HEAD               
6e73067adad0422ea76bbe18abaa38b571499622
$ GOOS=plan9 garble -literals build -o ./testdata/executable ./testdata      
# os
/tmp/garble-shared2885221212/os/file_plan9.go:131:15: invalid operation: cannot call non-function append (variable of type bool)
/tmp/garble-shared2885221212/os/file_plan9.go:139:15: invalid operation: cannot call non-function append (variable of type bool)
/tmp/garble-shared2885221212/os/file_plan9.go:142:15: invalid operation: cannot call non-function append (variable of type bool)
/tmp/garble-shared2885221212/os/file_plan9.go:145:15: invalid operation: cannot call non-function append (variable of type bool)
/tmp/garble-shared2885221212/os/file_plan9.go:147:15: invalid operation: cannot call non-function append (variable of type bool)
/tmp/garble-shared2885221212/os/file_plan9.go:150:15: invalid operation: cannot call non-function append (variable of type bool)
/tmp/garble-shared2885221212/os/file_plan9.go:166:40: invalid operation: cannot call non-function append (variable of type bool)
/tmp/garble-shared2885221212/os/file_plan9.go:188:14: invalid operation: cannot call non-function append (variable of type bool)
/tmp/garble-shared2885221212/os/file_plan9.go:190:14: invalid operation: cannot call non-function append (variable of type bool)
/tmp/garble-shared2885221212/os/file_plan9.go:194:14: invalid operation: cannot call non-function append (variable of type bool)
/tmp/garble-shared2885221212/os/file_plan9.go:194:14: too many errors
exit status 2
exit status 2
$ GOOS=plan9 garble -literals -tiny build -o ./testdata/executable ./testdata 
# os
/tmp/garble-shared1911714458/os/file_plan9.go:127:42: invalid operation: cannot call non-function append (variable of type bool)
/tmp/garble-shared1911714458/os/file_plan9.go:137:4: fullData declared but not used
/tmp/garble-shared1911714458/os/file_plan9.go:139:11: invalid operation: cannot call non-function append (variable of type bool)
/tmp/garble-shared1911714458/os/file_plan9.go:154:14: invalid operation: cannot call non-function append (variable of type bool)
/tmp/garble-shared1911714458/os/file_plan9.go:157:14: invalid operation: cannot call non-function append (variable of type bool)
/tmp/garble-shared1911714458/os/file_plan9.go:161:14: invalid operation: cannot call non-function append (variable of type bool)
/tmp/garble-shared1911714458/os/file_plan9.go:164:14: invalid operation: cannot call non-function append (variable of type bool)
exit status 2
exit status 2

@lu4p lu4p reopened this Nov 16, 2021
@mvdan
Copy link
Member

mvdan commented Nov 17, 2021

Oops, thanks for flagging. I was pretty sure that my fix was complete, but I didn't double-check with the full plan9 build.

mvdan added a commit to mvdan/garble-fork that referenced this issue Dec 5, 2021
Add a regression test in gogarble.txt,
as that test is already set up with packages to not obfuscate.

This bug manifested in the form of a build failure for GOOS=plan9
with -literals turned on:

	[...]/os/file_plan9.go:151:12: invalid operation: cannot call non-function append (variable of type bool)

In this case, the "os" package is not to be obfuscated,
but we would still obfuscate its literals as per the bug.

But, since the package's identifiers were not obfuscated,
names like "append" were not replaced as per ea2e0bd,
meaning that the shadowing would still affect us.

Fixes burrowers#417.
@mvdan mvdan closed this as completed in #428 Dec 6, 2021
mvdan added a commit that referenced this issue Dec 6, 2021
Add a regression test in gogarble.txt,
as that test is already set up with packages to not obfuscate.

This bug manifested in the form of a build failure for GOOS=plan9
with -literals turned on:

	[...]/os/file_plan9.go:151:12: invalid operation: cannot call non-function append (variable of type bool)

In this case, the "os" package is not to be obfuscated,
but we would still obfuscate its literals as per the bug.

But, since the package's identifiers were not obfuscated,
names like "append" were not replaced as per ea2e0bd,
meaning that the shadowing would still affect us.

Fixes #417.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants