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

panic: runtime error: invalid memory address or nil pointer dereference with garble, not with go #190

Closed
dustin-decker opened this issue Nov 9, 2020 · 9 comments
Labels
needs info Requires extra information to be solved

Comments

@dustin-decker
Copy link

When running a binary built with garble I'm getting:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x1c pc=0x40a23a]

goroutine 1 [running]:
google.golang.org/protobuf/internal/filetype.Builder.Build(0xbb078b, 0x9, 0x11dea20, 0xb22, 0xb22, 0x1300000003, 0x400000000, 0x0, 0xc6ed40, 0xc7d9c0, ...)
	google.golang.org/protobuf@v1.25.0/internal/filetype/build.go:131 +0xf4
zWqDrLXMm.zH6zbdznN()
	E.go:308 +0x1d8
zWqDrLXMm.init.0()
	2.go:100 +0x25

It runs fine without garble though.

$ go version
go version go1.15.2 linux/amd64
@mvdan
Copy link
Member

mvdan commented Nov 9, 2020

Thanks - can you provide any code to let us reproduce this crash? Otherwise it's going to be near impossible to debug and fix.

My guess is that, if you're using protobuf, it's somehow related to reflection.

@mvdan mvdan added the needs info Requires extra information to be solved label Nov 9, 2020
@dustin-decker
Copy link
Author

I'll see if I can reproduce with an example gRPC server.

@lu4p
Copy link
Member

lu4p commented Jan 20, 2021

@dustin-decker We need you to provide a case to reproduce, otherwise this issue will need to be closed as we cannot do anything here.

@lu4p
Copy link
Member

lu4p commented Feb 23, 2021

@dustin-decker Feel free to reopen once you share some code which reproduces this issue.

@lu4p lu4p closed this as completed Feb 23, 2021
@ghost
Copy link

ghost commented Feb 27, 2021

According to my test, when the enumerated type appears in protobuf, this error will occur after the binary compiled by garble build is executed.

golang version: go1.15.8 windows/amd64
garble version: v0.0.0-20210226105120-63c42c3cc746

main.go:

package main

import (
	"google.golang.org/protobuf/proto"
	"log"
	"test/protos" // my project name is "test"
)

func main() {
	data := []byte("")

	a := &protos.Test{}
	err := proto.Unmarshal(data, a)
	if err != nil {
		log.Println(err)
	} else {
		log.Println(a)
	}
	log.Println("Hello")
}

protos/test.proto:

syntax = "proto3";

package protos;
option go_package = "protos";

message Test {
  enum Type {
    A = 0;
    B = 1;
    C = 2;
    D = 3;
  }

  Type type = 1;

}

protos/test.pb.go:

// Code generated by protoc-gen-go. DO NOT EDIT.
// versions:
// 	protoc-gen-go v1.25.0
// 	protoc        v3.13.0
// source: protos/test.proto

package protos

import (
	proto "github.com/golang/protobuf/proto"
	protoreflect "google.golang.org/protobuf/reflect/protoreflect"
	protoimpl "google.golang.org/protobuf/runtime/protoimpl"
	reflect "reflect"
	sync "sync"
)

const (
	// Verify that this generated code is sufficiently up-to-date.
	_ = protoimpl.EnforceVersion(20 - protoimpl.MinVersion)
	// Verify that runtime/protoimpl is sufficiently up-to-date.
	_ = protoimpl.EnforceVersion(protoimpl.MaxVersion - 20)
)

// This is a compile-time assertion that a sufficiently up-to-date version
// of the legacy proto package is being used.
const _ = proto.ProtoPackageIsVersion4

type Test_Type int32

const (
	Test_A Test_Type = 0
	Test_B Test_Type = 1
	Test_C Test_Type = 2
	Test_D Test_Type = 3
)

// Enum value maps for Test_Type.
var (
	Test_Type_name = map[int32]string{
		0: "A",
		1: "B",
		2: "C",
		3: "D",
	}
	Test_Type_value = map[string]int32{
		"A": 0,
		"B": 1,
		"C": 2,
		"D": 3,
	}
)

func (x Test_Type) Enum() *Test_Type {
	p := new(Test_Type)
	*p = x
	return p
}

func (x Test_Type) String() string {
	return protoimpl.X.EnumStringOf(x.Descriptor(), protoreflect.EnumNumber(x))
}

func (Test_Type) Descriptor() protoreflect.EnumDescriptor {
	return file_protos_test_proto_enumTypes[0].Descriptor()
}

func (Test_Type) Type() protoreflect.EnumType {
	return &file_protos_test_proto_enumTypes[0]
}

func (x Test_Type) Number() protoreflect.EnumNumber {
	return protoreflect.EnumNumber(x)
}

// Deprecated: Use Test_Type.Descriptor instead.
func (Test_Type) EnumDescriptor() ([]byte, []int) {
	return file_protos_test_proto_rawDescGZIP(), []int{0, 0}
}

type Test struct {
	state         protoimpl.MessageState
	sizeCache     protoimpl.SizeCache
	unknownFields protoimpl.UnknownFields

	Type Test_Type `protobuf:"varint,1,opt,name=type,proto3,enum=protos.Test_Type" json:"type,omitempty"`
}

func (x *Test) Reset() {
	*x = Test{}
	if protoimpl.UnsafeEnabled {
		mi := &file_protos_test_proto_msgTypes[0]
		ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x))
		ms.StoreMessageInfo(mi)
	}
}

func (x *Test) String() string {
	return protoimpl.X.MessageStringOf(x)
}

func (*Test) ProtoMessage() {}

func (x *Test) ProtoReflect() protoreflect.Message {
	mi := &file_protos_test_proto_msgTypes[0]
	if protoimpl.UnsafeEnabled && x != nil {
		ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x))
		if ms.LoadMessageInfo() == nil {
			ms.StoreMessageInfo(mi)
		}
		return ms
	}
	return mi.MessageOf(x)
}

// Deprecated: Use Test.ProtoReflect.Descriptor instead.
func (*Test) Descriptor() ([]byte, []int) {
	return file_protos_test_proto_rawDescGZIP(), []int{0}
}

func (x *Test) GetType() Test_Type {
	if x != nil {
		return x.Type
	}
	return Test_A
}

var File_protos_test_proto protoreflect.FileDescriptor

var file_protos_test_proto_rawDesc = []byte{
	0x0a, 0x11, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x73, 0x2f, 0x74, 0x65, 0x73, 0x74, 0x2e, 0x70, 0x72,
	0x6f, 0x74, 0x6f, 0x12, 0x06, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x73, 0x22, 0x51, 0x0a, 0x04, 0x54,
	0x65, 0x73, 0x74, 0x12, 0x25, 0x0a, 0x04, 0x74, 0x79, 0x70, 0x65, 0x18, 0x01, 0x20, 0x01, 0x28,
	0x0e, 0x32, 0x11, 0x2e, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x73, 0x2e, 0x54, 0x65, 0x73, 0x74, 0x2e,
	0x54, 0x79, 0x70, 0x65, 0x52, 0x04, 0x74, 0x79, 0x70, 0x65, 0x22, 0x22, 0x0a, 0x04, 0x54, 0x79,
	0x70, 0x65, 0x12, 0x05, 0x0a, 0x01, 0x41, 0x10, 0x00, 0x12, 0x05, 0x0a, 0x01, 0x42, 0x10, 0x01,
	0x12, 0x05, 0x0a, 0x01, 0x43, 0x10, 0x02, 0x12, 0x05, 0x0a, 0x01, 0x44, 0x10, 0x03, 0x42, 0x08,
	0x5a, 0x06, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x73, 0x62, 0x06, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x33,
}

var (
	file_protos_test_proto_rawDescOnce sync.Once
	file_protos_test_proto_rawDescData = file_protos_test_proto_rawDesc
)

func file_protos_test_proto_rawDescGZIP() []byte {
	file_protos_test_proto_rawDescOnce.Do(func() {
		file_protos_test_proto_rawDescData = protoimpl.X.CompressGZIP(file_protos_test_proto_rawDescData)
	})
	return file_protos_test_proto_rawDescData
}

var file_protos_test_proto_enumTypes = make([]protoimpl.EnumInfo, 1)
var file_protos_test_proto_msgTypes = make([]protoimpl.MessageInfo, 1)
var file_protos_test_proto_goTypes = []interface{}{
	(Test_Type)(0), // 0: protos.Test.Type
	(*Test)(nil),   // 1: protos.Test
}
var file_protos_test_proto_depIdxs = []int32{
	0, // 0: protos.Test.type:type_name -> protos.Test.Type
	1, // [1:1] is the sub-list for method output_type
	1, // [1:1] is the sub-list for method input_type
	1, // [1:1] is the sub-list for extension type_name
	1, // [1:1] is the sub-list for extension extendee
	0, // [0:1] is the sub-list for field type_name
}

func init() { file_protos_test_proto_init() }
func file_protos_test_proto_init() {
	if File_protos_test_proto != nil {
		return
	}
	if !protoimpl.UnsafeEnabled {
		file_protos_test_proto_msgTypes[0].Exporter = func(v interface{}, i int) interface{} {
			switch v := v.(*Test); i {
			case 0:
				return &v.state
			case 1:
				return &v.sizeCache
			case 2:
				return &v.unknownFields
			default:
				return nil
			}
		}
	}
	type x struct{}
	out := protoimpl.TypeBuilder{
		File: protoimpl.DescBuilder{
			GoPackagePath: reflect.TypeOf(x{}).PkgPath(),
			RawDescriptor: file_protos_test_proto_rawDesc,
			NumEnums:      1,
			NumMessages:   1,
			NumExtensions: 0,
			NumServices:   0,
		},
		GoTypes:           file_protos_test_proto_goTypes,
		DependencyIndexes: file_protos_test_proto_depIdxs,
		EnumInfos:         file_protos_test_proto_enumTypes,
		MessageInfos:      file_protos_test_proto_msgTypes,
	}.Build()
	File_protos_test_proto = out.File
	file_protos_test_proto_rawDesc = nil
	file_protos_test_proto_goTypes = nil
	file_protos_test_proto_depIdxs = nil
}

@mvdan
Copy link
Member

mvdan commented Feb 27, 2021

Thanks @SNcoder, I can reproduce that panic.

@mvdan mvdan reopened this Feb 27, 2021
@ghost
Copy link

ghost commented Feb 27, 2021

I also found another error caused by using the dns library. I don't know if it is the same type of problem.

golang version: go1.15.8 windows/amd64
garble version: v0.0.0-20210226105120-63c42c3cc746

main.go:

package main

import (
	"github.com/miekg/dns"
	"log"
)

func main() {
	m1 := &dns.Msg{}
	m1.SetQuestion(dns.Fqdn("google.com"), dns.TypeA)
	in, err := dns.Exchange(m1, "8.8.8.8:53")
	if err != nil {
		log.Println(err)
	} else {
		log.Println(in)
	}
}

error message:

fatal error: unexpected signal during runtime execution
unexpected fault address 0xffffffffffffffff
fatal error: fault
[signal 0xc0000005 code=0x0 addr=0xffffffffffffffff pc=0x51bede]

goroutine 6 [running]:
runtime.throw(0x7af75f, 0x2a)
        runtime/panic.go:1116 +0x79 fp=0xc000037d70 sp=0xc000037d40 pc=0x548379
runtime.sigpanic()
        runtime/signal_windows.go:240 +0x285 fp=0xc000037da0 sp=0xc000037d70 pc=0x55b685
runtime.lock2(0x6620203a656e69c4)
        runtime/lock_sema.go:47 +0x5e fp=0xc000037dd8 sp=0xc000037da0 pc=0x51bede
runtime.lockWithRank(...)
        runtime/lockrank_off.go:28
runtime.lock(...)
        runtime/lock_sema.go:36
runtime.sellock(0xc000037f60, 0x2, 0x2, 0xc000037f5c, 0x2, 0x2)
        runtime/select.go:52 +0x85 fp=0xc000037e08 sp=0xc000037dd8 pc=0x558565
runtime.selectgo(0xc000037f60, 0xc000037f58, 0x2, 0x0, 0x0)
        runtime/select.go:225 +0x3bd fp=0xc000037f30 sp=0xc000037e08 pc=0x558b1d
net.(*Dialer).DialContext.func1(0x6620203a656e696c, 0xc00002e6a0, 0x7ee4e0, 0xc000038040)
        net/dial.go:385 +0xbe fp=0xc000037fc0 sp=0xc000037f30 pc=0x66ee7e
runtime.goexit()
        runtime/asm_amd64.s:1374 +0x1 fp=0xc000037fc8 sp=0xc000037fc0 pc=0x579461
created by net.(*Dialer).DialContext
        net/dial.go:384 +0x95e

goroutine 1 [running]:
        goroutine running on other thread; stack unavailable
[signal 0xc0000005 code=0x0 addr=0xffffffffffffffff pc=0x6576a3]

goroutine 1 [running]:
runtime.throw(0x7a34a5, 0x5)
        runtime/panic.go:1116 +0x79 fp=0xc000075ad0 sp=0xc000075aa0 pc=0x548379
runtime.sigpanic()
        runtime/signal_windows.go:249 +0x24f fp=0xc000075b00 sp=0xc000075ad0 pc=0x55b64f
net.(*Resolver).resolveAddrList(0x207461203d94ce6c, 0x7ee4e0, 0xc000038040, 0x7a3266, 0x4, 0x7a30f2, 0x3, 0x7a4e06, 0xc, 0x676c41205b3d6b63, ...)
        net/dial.go:231 +0x543 fp=0xc000075c10 sp=0xc000075b00 pc=0x6576a3
net.(*Dialer).DialContext(0xc000075dc8, 0x7ee520, 0xc00000c0b8, 0x7a30f2, 0x3, 0x7a4e06, 0xc, 0x0, 0x0, 0x0, ...)
        net/dial.go:403 +0x23c fp=0xc000075d50 sp=0xc000075c10 pc=0x65857c
net.(*Dialer).Dial(...)
        net/dial.go:348
github.com/miekg/dns.(*Client).Dial(0xc00011c000, 0x7a4e06, 0xc, 0xc000004700, 0xc000075e88, 0x51e67f)
        github.com/miekg/dns@v1.1.35/client.go:104 +0x325 fp=0xc000075e38 sp=0xc000075d50 pc=0x6d7dc5
github.com/miekg/dns.(*Client).Exchange(0xc00011c000, 0xc000075ef0, 0x7a4e06, 0xc, 0x0, 0x0, 0x0, 0x0)
        github.com/miekg/dns@v1.1.35/client.go:129 +0x7a fp=0xc000075e98 sp=0xc000075e38 pc=0x6d7f7a
github.com/miekg/dns.Exchange(...)
        github.com/miekg/dns@v1.1.35/client.go:53
main.main()
        t.go:5 +0x10b fp=0xc000075f88 sp=0xc000075e98 pc=0x74386b
runtime.main()
        runtime/proc.go:204 +0x209 fp=0xc000075fe0 sp=0xc000075f88 pc=0x54ab29
runtime.goexit()
        runtime/asm_amd64.s:1374 +0x1 fp=0xc000075fe8 sp=0xc000075fe0 pc=0x579461

@mvdan
Copy link
Member

mvdan commented Feb 27, 2021

If I disable import path obfuscation, the protobuf segfault goes away. The obfuscated code also seems correct to me. It's possible that we are breaking the compiled object file.

@mvdan
Copy link
Member

mvdan commented Feb 27, 2021

I can also confirm this is not a recent regression; e014f48 has the same bug.

mvdan added a commit to mvdan/garble-fork that referenced this issue Feb 27, 2021
We used to rely on a parallel implementation of an object file parser
and writer to be able to obfuscate import paths. After compiling each
package, we would parse the object file, replace the import paths, and
write the updated object file in-place.

That worked well, in most cases. Unfortunately, it had some flaws:

* Complexity. Even when most of the code is maintained in a separate
  module, the import_obfuscation.go file was still close to a thousand
  lines of code.

* Go compatibility. The object file format changes between Go releases,
  so we were supporting Go 1.15, but not 1.16. Fixing the object file
  package to work with 1.16 would probably break 1.15 support.

* Bugs. For example, we recently had to add a workaround for burrowers#224, since
  import paths containing dots after the domain would end up escaped.
  Another example is burrowers#190, which seems to be caused by the object file
  parser or writer corrupting the compiled code and causing segfaults in
  some rare edge cases.

Instead, let's drop that method entirely, and force the compiler and
linker to do the work for us. The steps necessary when compiling a
package to obfuscate are:

1) Replace its "package foo" line with the obfuscated package path. No
   need to separate the package path and name, since the obfuscated path
   does not contain slashes.

2) Replace the "-p pkg/foo" flag with the obfuscated path.

3) Replace the "-importcfg [...]" file with a version that uses the
   obfuscated paths instead.

The linker also needs that last step, since it also uses an importcfg
file to find object files.

There are three noteworthy drawbacks to this new method:

1) Since we no longer write object files, we can't use them to store
   data to be cached. As such, the -debugdir flag goes back to using the
   "-a" build flag to always rebuild all packages. On the plus side,
   that caching didn't work very well; see burrowers#176.

2) The package name "main" remains in all declarations under it, not
   just "func main", since we can only rename entire packages. This
   seems fine, as it gives little information to the end user.

3) The -tiny mode no longer sets all lines to 0, since it did that by
   modifying object files. As a temporary measure, we instead set all
   top-level declarations to be on line 1. A TODO is added to hopefully
   improve this again in the near future.

The upside is that we get rid of all the issues mentioned before. Plus,
garble now nearly works with Go 1.16, with the exception of two very
minor bugs that look fixable. A follow-up PR will take care of that and
start testing on 1.16.

Fixes burrowers#176.
Fixes burrowers#190.
mvdan added a commit to mvdan/garble-fork that referenced this issue Feb 27, 2021
We used to rely on a parallel implementation of an object file parser
and writer to be able to obfuscate import paths. After compiling each
package, we would parse the object file, replace the import paths, and
write the updated object file in-place.

That worked well, in most cases. Unfortunately, it had some flaws:

* Complexity. Even when most of the code is maintained in a separate
  module, the import_obfuscation.go file was still close to a thousand
  lines of code.

* Go compatibility. The object file format changes between Go releases,
  so we were supporting Go 1.15, but not 1.16. Fixing the object file
  package to work with 1.16 would probably break 1.15 support.

* Bugs. For example, we recently had to add a workaround for burrowers#224, since
  import paths containing dots after the domain would end up escaped.
  Another example is burrowers#190, which seems to be caused by the object file
  parser or writer corrupting the compiled code and causing segfaults in
  some rare edge cases.

Instead, let's drop that method entirely, and force the compiler and
linker to do the work for us. The steps necessary when compiling a
package to obfuscate are:

1) Replace its "package foo" lines with the obfuscated package path. No
   need to separate the package path and name, since the obfuscated path
   does not contain slashes.

2) Replace the "-p pkg/foo" flag with the obfuscated path.

3) Replace the "import" spec lines with the obfuscated package paths,
   for those dependencies which were obfuscated.

4) Replace the "-importcfg [...]" file with a version that uses the
   obfuscated paths instead.

The linker also needs that last step, since it also uses an importcfg
file to find object files.

There are three noteworthy drawbacks to this new method:

1) Since we no longer write object files, we can't use them to store
   data to be cached. As such, the -debugdir flag goes back to using the
   "-a" build flag to always rebuild all packages. On the plus side,
   that caching didn't work very well; see burrowers#176.

2) The package name "main" remains in all declarations under it, not
   just "func main", since we can only rename entire packages. This
   seems fine, as it gives little information to the end user.

3) The -tiny mode no longer sets all lines to 0, since it did that by
   modifying object files. As a temporary measure, we instead set all
   top-level declarations to be on line 1. A TODO is added to hopefully
   improve this again in the near future.

The upside is that we get rid of all the issues mentioned before. Plus,
garble now nearly works with Go 1.16, with the exception of two very
minor bugs that look fixable. A follow-up PR will take care of that and
start testing on 1.16.

Fixes burrowers#176.
Fixes burrowers#190.
@lu4p lu4p closed this as completed in 05d0dd1 Feb 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs info Requires extra information to be solved
Development

No branches or pull requests

3 participants