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.

Sign up for GitHub

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

Jump to bottom

fix: hold_body_chunk should use seperate buffer for each plugin in case of pollution #9266

Merged
merged 3 commits into from
Apr 13, 2023
Merged

fix: hold_body_chunk should use seperate buffer for each plugin in case of pollution #9266

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8b8fbfe
fix: hold_body_chunk can not be used in more than one plugin
Sn0rt Apr 7, 2023
7a6ea8a
fix: grpc-transcode2.t logs duplicate
Sn0rt Apr 10, 2023
82b13dc
Merge branch 'master' into guohao/bugfix-9228
Sn0rt Apr 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions apisix/core/response.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,14 +177,19 @@ end
function _M.hold_body_chunk(ctx, hold_the_copy)
local body_buffer
local chunk, eof = arg[1], arg[2]

if not ctx._body_buffer then
ctx._body_buffer = {}
end

if type(chunk) == "string" and chunk ~= "" then
body_buffer = ctx._body_buffer
body_buffer = ctx._body_buffer[ctx._plugin_name]
if not body_buffer then
body_buffer = {
chunk,
n = 1
}
ctx._body_buffer = body_buffer
ctx._body_buffer[ctx._plugin_name] = body_buffer
else
local n = body_buffer.n + 1
body_buffer.n = n
Expand All @@ -193,13 +198,13 @@ function _M.hold_body_chunk(ctx, hold_the_copy)
end

if eof then
body_buffer = ctx._body_buffer
body_buffer = ctx._body_buffer[ctx._plugin_name]
if not body_buffer then
return chunk
end

body_buffer = concat_tab(body_buffer, "", 1, body_buffer.n)
ctx._body_buffer = nil
ctx._body_buffer[ctx._plugin_name] = nil
return body_buffer
end

Expand Down
4 changes: 4 additions & 0 deletions apisix/plugin.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1093,7 +1093,9 @@ function _M.run_plugin(phase, plugins, api_ctx)
end

plugin_run = true
api_ctx._plugin_name = plugins[i]["name"]
local code, body = phase_func(conf, api_ctx)
api_ctx._plugin_name = nil
if code or body then
if is_http then
if code >= 400 then
Expand Down Expand Up @@ -1128,7 +1130,9 @@ function _M.run_plugin(phase, plugins, api_ctx)
local conf = plugins[i + 1]
if phase_func and meta_filter(api_ctx, plugins[i]["name"], conf) then
plugin_run = true
api_ctx._plugin_name = plugins[i]["name"]
phase_func(conf, api_ctx)
api_ctx._plugin_name = nil
end
end

Expand Down
1 change: 1 addition & 0 deletions t/core/response.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@ aaa:
}
body_filter_by_lua_block {
local core = require("apisix.core")
ngx.ctx._plugin_name = "test"
local final_body = core.response.hold_body_chunk(ngx.ctx)
if not final_body then
return
Expand Down
2 changes: 1 addition & 1 deletion t/plugin/grpc-transcode2.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,7 @@ GET /grpc_plus?a=1&b=2
--- response_body eval
qr/\{"result":3\}/
--- error_log eval
qr/request log: \{.*body":\"\\u0000\\u0000\\u0000\\u0000\\u0002\\b\\u0003\\u0000\\u0000\\u0000\\u0000\\u0002\\b\\u0003"/
qr/request log: \{.*body":\"\\u0000\\u0000\\u0000\\u0000\\u0002\\b\\u0003"/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR fixes another bug: insert the same chunk twice (by different plugins) into the body_buffer.

Bug reproduce:

diff --git a/apisix/core/response.lua b/apisix/core/response.lua
index b934d94b..971961b3 100644
--- a/apisix/core/response.lua
+++ b/apisix/core/response.lua
@@ -178,6 +178,8 @@ function _M.hold_body_chunk(ctx, hold_the_copy)
     local body_buffer
     local chunk, eof = arg[1], arg[2]
     if type(chunk) == "string" and chunk ~= "" then
+        ngx.log(ngx.WARN, "#chunk: ", #chunk)
+        ngx.log(ngx.WARN, debug.traceback("hold_body_chunk", 3))
         body_buffer = ctx._body_buffer
         if not body_buffer then
             body_buffer = {
@@ -185,10 +187,13 @@ function _M.hold_body_chunk(ctx, hold_the_copy)
                 n = 1
             }
             ctx._body_buffer = body_buffer
+            ngx.log(ngx.WARN, "create body_buffer: ", tostring(body_buffer))
+            ngx.log(ngx.WARN, "init #chunk: ", #chunk)
         else
             local n = body_buffer.n + 1
             body_buffer.n = n
             body_buffer[n] = chunk
+            ngx.log(ngx.WARN, "add #chunk: ", #chunk)
         end
     end

error.log from this test case:

2023/04/12 18:08:06 [warn] 2785045#2785045: *3 [lua] response.lua:181: hold_body_chunk(): #chunk: 7 while sending to client, client: 127.0.0.1, server: localhost, request: "GET /grpc_plus?a=1&b=2 HTTP/1.1", upstream: "grpc://127.0.0.1:50051", host: "localhost"
2023/04/12 18:08:06 [warn] 2785045#2785045: *3 [lua] response.lua:182: hold_body_chunk(): hold_body_chunk
stack traceback:
        /opt/apisix/apisix/plugins/http-logger.lua:153: in function 'phase_func'
        /opt/apisix/apisix/plugin.lua:1131: in function 'run_plugin'
        /opt/apisix/apisix/plugin.lua:1164: in function 'run_global_rules'
        /opt/apisix/apisix/init.lua:403: in function 'common_phase'
        /opt/apisix/apisix/init.lua:747: in function 'http_body_filter_phase'
        body_filter_by_lua:2: in main chunk while sending to client, client: 127.0.0.1, server: localhost, request: "GET /grpc_plus?a=1&b=2 HTTP/1.1", upstream: "grpc://127.0.0.1:50051", host: "localhost"
2023/04/12 18:08:06 [warn] 2785045#2785045: *3 [lua] response.lua:190: hold_body_chunk(): create body_buffer: table: 0x7f4aede1e4c8 while sending to client, client: 127.0.0.1, server: localhost, request: "GET /grpc_plus?a=1&b=2 HTTP/1.1", upstream: "grpc://127.0.0.1:50051", host: "localhost"
2023/04/12 18:08:06 [warn] 2785045#2785045: *3 [lua] response.lua:191: hold_body_chunk(): init #chunk: 7 while sending to client, client: 127.0.0.1, server: localhost,request: "GET /grpc_plus?a=1&b=2 HTTP/1.1", upstream: "grpc://127.0.0.1:50051", host: "localhost"
2023/04/12 18:08:06 [warn] 2785045#2785045: *3 [lua] response.lua:181: hold_body_chunk(): #chunk: 7 while sending to client, client: 127.0.0.1, server: localhost, request: "GET /grpc_plus?a=1&b=2 HTTP/1.1", upstream: "grpc://127.0.0.1:50051", host: "localhost"
2023/04/12 18:08:06 [warn] 2785045#2785045: *3 [lua] response.lua:182: hold_body_chunk(): hold_body_chunk
stack traceback:
        /opt/apisix/apisix/plugins/grpc-transcode.lua:202: in function 'phase_func'
        /opt/apisix/apisix/plugin.lua:1131: in function 'common_phase'
        /opt/apisix/apisix/init.lua:747: in function 'http_body_filter_phase'
        body_filter_by_lua:2: in main chunk while sending to client, client: 127.0.0.1, server: localhost, request: "GET /grpc_plus?a=1&b=2 HTTP/1.1", upstream: "grpc://127.0.0.1:50051", host: "localhost"
2023/04/12 18:08:06 [warn] 2785045#2785045: *3 [lua] response.lua:196: hold_body_chunk(): add #chunk: 7 while sending to client, client: 127.0.0.1, server: localhost, request: "GET /grpc_plus?a=1&b=2 HTTP/1.1", upstream: "grpc://127.0.0.1:50051", host: "localhost"
2023/04/12 18:08:06 [warn] 2785045#2785045: *3 [lua] response.lua:110: response(): #buffer: 14 while sending to client, client: 127.0.0.1, server: localhost, request: "GET /grpc_plus?a=1&b=2 HTTP/1.1", upstream: "grpc://127.0.0.1:50051", host: "localhost"




Expand Down