fix: avoid reusing nil builder #42

spacewander · 2021-10-08T03:54:45Z

spacewander · 2021-10-08T04:01:41Z

@a11enhuang @envestcc
Could you check if this fixes the bug?

envestcc · 2021-10-08T06:23:35Z

@a11enhuang @envestcc Could you check if this fixes the bug?

I have integrated this commit into my project and deployed into testing environment. Because it will take some time to reproduce this problem, i will reply maybe in tomorrow.

a11enhuang · 2021-10-08T06:31:11Z

This problem still exists today.

…

------------------ 原始邮件 ------------------ 发件人: "apache/apisix-go-plugin-runner" ***@***.***>; 发送时间: 2021年10月8日(星期五) 下午2:23 ***@***.***>; ***@***.******@***.***>; 主题: Re: [apache/apisix-go-plugin-runner] fix: avoid reusing nil builder (#42) @a11enhuang @envestcc Could you check if this fixes the bug? I have integrated this commit into my project and deployed into testing environment. Because it will take some time to reproduce this problem, i will reply maybe in tomorrow. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.

a11enhuang · 2021-10-08T06:33:55Z

This problem still exists today.I tried to merge the code from master，but the problem is still not fixed.

…

------------------ 原始邮件 ------------------ 发件人: "apache/apisix-go-plugin-runner" ***@***.***>; 发送时间: 2021年10月8日(星期五) 中午12:01 ***@***.***>; ***@***.******@***.***>; 主题: Re: [apache/apisix-go-plugin-runner] fix: avoid reusing nil builder (#42) @a11enhuang @envestcc Could you check if this fixes the bug? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.

spacewander · 2021-10-08T08:14:20Z

@a11enhuang
What's the new stack trace?

a11enhuang · 2021-10-08T09:06:37Z

This is the new stack trace.

…

2021年10月8日 16:14，罗泽轩 ***@***.***> 写道： @a11enhuang <https://github.com/A11enHuang> What's the new stack trace? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#42 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALWW7KO2VX3JBQX53K3LRYTUF2R6NANCNFSM5FSWBSYQ>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

spacewander · 2021-10-08T09:11:53Z

@a11enhuang
Err... Don't reply image in the email, GitHub only shows the text part.

a11enhuang · 2021-10-08T09:15:37Z

sorry. 2021/10/08 07:46:32 [warn] 56#56: *207 [lua] init.lua:681: 2021-10-08T07:46:32.966Z ERROR server/server.go:69 panic recovered: runtime error: invalid memory address or nil pointer dereference  github.com/apache/apisix-go-plugin-runner/internal/server.recoverPanic  /var/lib/jenkins/workspace/build-dev-apple-fuller-apisix-go-plugin-runner/apisix-go-plugin-runner/internal/server/server.go:69  runtime.gopanic  /root/go/src/runtime/panic.go:1038  runtime.panicmem  /root/go/src/runtime/panic.go:221  runtime.sigpanic  /root/go/src/runtime/signal_unix.go:735  github.com/google/flatbuffers/go.(*Builder).Reset  ***@***.***+incompatible/go/builder.go:45  github.com/apache/apisix-go-plugin-runner/internal/util.PutBuilder  /var/lib/jenkins/workspace/build-dev-apple-fuller-apisix-go-plugin-runner/apisix-go-plugin-runner/internal/util/pool.go:37  github.com/apache/apisix-go-plugin-runner/internal/server.handleConn  /var/lib/jenkins/workspace/build-dev-apple-fuller-apisix-go-plugin-runner/apisix-go-plugin-runner/internal/server/server.go:122  , context: ngx.timer 

…

------------------ 原始邮件 ------------------ 发件人: "apache/apisix-go-plugin-runner" ***@***.***>; 发送时间: 2021年10月8日(星期五) 下午5:12 ***@***.***>; ***@***.******@***.***>; 主题: Re: [apache/apisix-go-plugin-runner] fix: avoid reusing nil builder (#42) @a11enhuang Err... Don't reply image in the email, GitHub only shows the text part. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.

spacewander · 2021-10-08T09:33:17Z

Interesting. We don't call util.PutBuilder in server/server.go:122 after applying this change.

Here is the old stack:

2021/10/8 上午11:03:14 github.com/apache/apisix-go-plugin-runner/internal/util.PutBuilder
2021/10/8 上午11:03:14 /var/lib/jenkins/workspace/build-dev-apple-fuller-apisix-go-plugin-runner/apisix-go-plugin-runner/internal/util/pool.go:37
2021/10/8 上午11:03:14 github.com/apache/apisix-go-plugin-runner/internal/server.handleConn
2021/10/8 上午11:03:14 /var/lib/jenkins/workspace/build-dev-apple-fuller-apisix-go-plugin-runner/apisix-go-plugin-runner/internal/server/server.go:122

Here is the new stack:

github.com/apache/apisix-go-plugin-runner/internal/util.PutBuilder&nbsp;
	/var/lib/jenkins/workspace/build-dev-apple-fuller-apisix-go-plugin-runner/apisix-go-plugin-runner/internal/util/pool.go:37&nbsp;
github.com/apache/apisix-go-plugin-runner/internal/server.handleConn&nbsp;
	/var/lib/jenkins/workspace/build-dev-apple-fuller-apisix-go-plugin-runner/apisix-go-plugin-runner/internal/server/server.go:122

Could you check if the code has the given patch? I already removed the util.PutBuilder in line 122.

diff --git a/internal/server/server.go b/internal/server/server.go
index 48d6da0..6193a11 100644
--- a/internal/server/server.go
+++ b/internal/server/server.go
@@ -70,12 +70,22 @@ func recoverPanic() {
 	}
 }
 
-func dispatchRPC(ty byte, in []byte, conn net.Conn) (*flatbuffers.Builder, error) {
+func dispatchRPC(ty byte, in []byte, conn net.Conn) *flatbuffers.Builder {
+	var err error
+	var bd *flatbuffers.Builder
 	hl, ok := typeHandlerMap[ty]
 	if !ok {
-		return nil, UnknownType{ty}
+		err = UnknownType{ty}
+	} else {
+		bd, err = hl(in, conn)
+	}
+
+	if err != nil {
+		bd = generateErrorReport(err)
+	} else {
+		bd = checkIfDataTooLarge(bd)
 	}
-	return hl(in, conn)
+	return bd
 }
 
 func checkIfDataTooLarge(bd *flatbuffers.Builder) *flatbuffers.Builder {
@@ -116,15 +126,7 @@ func handleConn(c net.Conn) {
 			break
 		}
 
-		bd, err := dispatchRPC(ty, buf, c)
-
-		if err != nil {
-			util.PutBuilder(bd)
-			bd = generateErrorReport(err)
-		} else {
-			bd = checkIfDataTooLarge(bd)
-		}
-
+		bd := dispatchRPC(ty, buf, c)
 		out := bd.FinishedBytes()
 		size := len(out)
 		binary.BigEndian.PutUint32(header, uint32(size))

envestcc · 2021-10-08T09:33:32Z

@a11enhuang @envestcc Could you check if this fixes the bug?

I have integrated this commit into my project and deployed into testing environment. Because it will take some time to reproduce this problem, i will reply maybe in tomorrow.

Until now, the panic did not happen again. but sometimes the request is directly write to upstream, don't pass to my external-go-plugin, with only a log msg like "WARN server/server.go:59 key not found"

spacewander · 2021-10-08T09:47:58Z

@envestcc
Let's discuss the issue there: #43

a11enhuang · 2021-10-09T01:44:48Z

Today I will use the code of the master branch to confirm whether the problem is fixed.

…

2021年10月8日 17:33，罗泽轩 ***@***.***> 写道： Interesting. We don't call util.PutBuilder in server/server.go:122 after applying this change. Here is the old stack: 2021/10/8 上午11:03:14 github.com/apache/apisix-go-plugin-runner/internal/util.PutBuilder 2021/10/8 上午11:03:14 /var/lib/jenkins/workspace/build-dev-apple-fuller-apisix-go-plugin-runner/apisix-go-plugin-runner/internal/util/pool.go:37 2021/10/8 上午11:03:14 github.com/apache/apisix-go-plugin-runner/internal/server.handleConn 2021/10/8 上午11:03:14 /var/lib/jenkins/workspace/build-dev-apple-fuller-apisix-go-plugin-runner/apisix-go-plugin-runner/internal/server/server.go:122 Here is the new stack: github.com/apache/apisix-go-plugin-runner/internal/util.PutBuilder  /var/lib/jenkins/workspace/build-dev-apple-fuller-apisix-go-plugin-runner/apisix-go-plugin-runner/internal/util/pool.go:37  github.com/apache/apisix-go-plugin-runner/internal/server.handleConn  /var/lib/jenkins/workspace/build-dev-apple-fuller-apisix-go-plugin-runner/apisix-go-plugin-runner/internal/server/server.go:122 Could you check if the code has the given patch? I already removed the util.PutBuilder in line 122. diff --git a/internal/server/server.go b/internal/server/server.go index 48d6da0..6193a11 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -70,12 +70,22 @@ func recoverPanic() { } } -func dispatchRPC(ty byte, in []byte, conn net.Conn) (*flatbuffers.Builder, error) { +func dispatchRPC(ty byte, in []byte, conn net.Conn) *flatbuffers.Builder { + var err error + var bd *flatbuffers.Builder hl, ok := typeHandlerMap[ty] if !ok { - return nil, UnknownType{ty} + err = UnknownType{ty} + } else { + bd, err = hl(in, conn) + } + + if err != nil { + bd = generateErrorReport(err) + } else { + bd = checkIfDataTooLarge(bd) } - return hl(in, conn) + return bd } func checkIfDataTooLarge(bd *flatbuffers.Builder) *flatbuffers.Builder { @@ -116,15 +126,7 @@ func handleConn(c net.Conn) { break } - bd, err := dispatchRPC(ty, buf, c) - - if err != nil { - util.PutBuilder(bd) - bd = generateErrorReport(err) - } else { - bd = checkIfDataTooLarge(bd) - } - + bd := dispatchRPC(ty, buf, c) out := bd.FinishedBytes() size := len(out) binary.BigEndian.PutUint32(header, uint32(size)) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#42 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALWW7KIEAQF26OXYN7JGLWLUF23GRANCNFSM5FSWBSYQ>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

spacewander · 2021-10-09T04:23:58Z

@a11enhuang
I just merge this change into the master. Please ensure the commit hash is 9b2683c and its content contains the change.

fix: avoid reusing nil builder

5f7f879

Fix apache#41

tokers approved these changes Oct 8, 2021

View reviewed changes

spacewander merged commit 9b2683c into apache:master Oct 9, 2021

spacewander deleted the fsv branch October 9, 2021 04:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: avoid reusing nil builder #42

fix: avoid reusing nil builder #42

spacewander commented Oct 8, 2021

spacewander commented Oct 8, 2021

envestcc commented Oct 8, 2021

a11enhuang commented Oct 8, 2021 via email

a11enhuang commented Oct 8, 2021 via email

spacewander commented Oct 8, 2021

a11enhuang commented Oct 8, 2021 via email

spacewander commented Oct 8, 2021

a11enhuang commented Oct 8, 2021 via email

spacewander commented Oct 8, 2021

envestcc commented Oct 8, 2021

spacewander commented Oct 8, 2021

a11enhuang commented Oct 9, 2021 via email

spacewander commented Oct 9, 2021

fix: avoid reusing nil builder #42

fix: avoid reusing nil builder #42

Conversation

spacewander commented Oct 8, 2021

spacewander commented Oct 8, 2021

envestcc commented Oct 8, 2021

a11enhuang commented Oct 8, 2021 via email

a11enhuang commented Oct 8, 2021 via email

spacewander commented Oct 8, 2021

a11enhuang commented Oct 8, 2021 via email

spacewander commented Oct 8, 2021

a11enhuang commented Oct 8, 2021 via email

spacewander commented Oct 8, 2021

envestcc commented Oct 8, 2021

spacewander commented Oct 8, 2021

a11enhuang commented Oct 9, 2021 via email

spacewander commented Oct 9, 2021