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

fix(env) luajit table overflow error when using ffi.cdef #8945

Merged
merged 1 commit into from
Jun 14, 2022
Merged

Conversation

ms2008
Copy link
Contributor

@ms2008 ms2008 commented Jun 14, 2022

Summary

There is a potential risk that if init is invoked a lot, a table overflow error will occur. Can be verified very simply by:

$ resty -e 'local ffi = require "ffi" while true do ffi.cdef[[void exit(int status);]] end'
ERROR: table overflow
stack traceback:
        (command line -e):1: in function 'inline_gen'
        init_worker_by_lua:44: in function <init_worker_by_lua:43>
        [C]: in function 'xpcall'
        init_worker_by_lua:52: in function <init_worker_by_lua:50>

It's wrong to call ffi.cdef() repeatedly for the same things. Always call it on the top-level scope of module files.

Full changelog

  • fix luajit table overflow error when using ffi.cdef

@ms2008 ms2008 requested a review from a team as a code owner June 14, 2022 07:05
@ms2008 ms2008 requested a review from bungle June 14, 2022 07:05
@bungle
Copy link
Member

bungle commented Jun 14, 2022

@ms2008, init is only ever executed once (in init phase of nginx).

@ms2008
Copy link
Contributor Author

ms2008 commented Jun 14, 2022

@bungle Sorry, I am not very familiar with the valut thing. I see it being called here:

kong/kong/pdk/vault.lua

Lines 163 to 165 in 802e1aa

if strategy.init then
strategy.init()
end

will eventually be called by this method:

function _VAULT.get(reference)

It looks like it might be called multiple times if I understand correctly:

kong/kong/db/schema/init.lua

Lines 1744 to 1760 in 802e1aa

for i = 1, count do
if is_reference(value[i]) then
refs[key][i] = value[i]
local deref, err = kong.vault.get(value[i])
if deref then
value[i] = deref
else
if err then
kong.log.warn("unable to resolve reference ", value[i], " (", err, ")")
else
kong.log.warn("unable to resolve reference ", value[i])
end
value[i] = nil
end
end
end

@bungle
Copy link
Member

bungle commented Jun 14, 2022

@bungle Sorry, I am not very familiar with the valut thing. I see it being called here:

kong/kong/pdk/vault.lua

Lines 163 to 165 in 802e1aa

if strategy.init then
strategy.init()
end

will eventually be called by this method:

Good point. Let me check this too. The init might not be needed there, let me see. This is quite complicated as it has to work on CLI etc.

@bungle bungle merged commit 4f70b29 into master Jun 14, 2022
@bungle bungle deleted the fix/env branch June 14, 2022 09:54
bungle added a commit that referenced this pull request Jun 15, 2022
### Summary

@ms2008 pointed on PR #8945 that we may have potential issue in Vault that
init method or vault strategy (plugin) may be called multiple times. This
commit makes is sure that it should not happen anymore.
bungle added a commit that referenced this pull request Jun 15, 2022
### Summary

@ms2008 pointed on PR #8945 that we may have potential issue in Vault that
init method or vault strategy (plugin) may be called multiple times. This
commit makes is sure that it should not happen anymore.
bungle added a commit that referenced this pull request Jun 15, 2022
### Summary

@ms2008 pointed on PR #8945 that we may have potential issue in Vault that
init method or vault strategy (plugin) may be called multiple times. This
commit makes is sure that it should not happen anymore.
bungle added a commit that referenced this pull request Jun 15, 2022
### Summary

@ms2008 pointed on PR #8945 that we may have potential issue in Vault that
init method or vault strategy (plugin) may be called multiple times. This
commit makes is sure that it should not happen anymore.
locao pushed a commit that referenced this pull request Jun 21, 2024
FTI-5896

---------

Co-authored-by: Qi <add_sp@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants