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

lua: prevent serialization of error for ucdata #9413

Merged

Conversation

Buristan
Copy link
Collaborator

The alternative solution is to use just lua_call() instead, since the error should be caught somewhere in the upper frames.

diff --git a/src/lua/serializer.c b/src/lua/serializer.c
index 1018f9572..56400e7f3 100644
--- a/src/lua/serializer.c
+++ b/src/lua/serializer.c
@@ -274,7 +274,11 @@ lua_field_inspect_ucdata(struct lua_State *L, struct luaL_serializer *cfg,
                 }
                 /* copy object itself */
                 lua_pushvalue(L, idx);
-                lua_pcall(L, 1, 1, 0);
+                /*
+                 * The possible error should be catched in some
+                 * upper frame.
+                 */
+                lua_call(L, 1, 1);
                 /* replace obj with the unpacked value */
                 lua_replace(L, idx);
                 if (luaL_tofield(L, cfg, idx, field) < 0)

The results of the benchmark below for both ways are quite similar, so I prefer the most common way (the one in the patch):

time src/tarantool -e "                                                                
local yml = require'yaml'
local ud_mt = {__serialize = function()
  return ''
end}
ud_mt.__index = ud_mt
local ud = newproxy(true)
debug.setmetatable(ud, ud_mt)
collectgarbage('stop')
for _ = 1, 3e6 do
    yml.encode(ud)
end
"  
lua_pcall() lua_call()
$4.896s\ (\sigma=0.037$) $4.879s\ (\sigma=0.034$)

Without checking the return value of lua_pcall()` in
`lua_field_inspect_ucdata()`, the error message itself is returned as a
serialized result. The result status of `lua_pcall()` is not ignored
now.

NO_DOC=bugfix

Closes tarantool#9396
@Buristan Buristan added lua serializers Fearutes and bugs related to built-in serializers labels Nov 27, 2023
@coveralls
Copy link

Coverage Status

coverage: 86.688% (+0.04%) from 86.644%
when pulling 2a57129 on Buristan:skaplun/gh-9396-ucdata-serialize-error
into 3ee68d8
on tarantool:master
.

@Buristan Buristan added the full-ci Enables all tests for a pull request label Nov 27, 2023
@Buristan Buristan marked this pull request as ready for review November 27, 2023 10:55
@Buristan Buristan requested review from a team as code owners November 27, 2023 10:55
Copy link
Collaborator

@igormunkin igormunkin left a comment

Choose a reason for hiding this comment

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

@Buristan, thanks for the patch! LGTM.

@igormunkin igormunkin removed their assignment Nov 29, 2023
Copy link
Contributor

@Lord-KA Lord-KA left a comment

Choose a reason for hiding this comment

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

Thanks for the patch. I don't see any issues with the change, LGTM.

Nit: @Buristan, I am just curious, why did you leave the XXX in the test comments?

@Lord-KA Lord-KA assigned igormunkin and unassigned Lord-KA Nov 30, 2023
@Buristan
Copy link
Collaborator Author

@Lord-KA,
AFAIK, XXX is just a legacy keyword coming from Sun code convention:

10.5.4 Special Comments
Use XXX in a comment to flag something that is bogus but works. Use FIXME to flag something that is bogus and broken.

@igormunkin igormunkin merged commit 98474f7 into tarantool:master Nov 30, 2023
143 of 146 checks passed
@Lord-KA
Copy link
Contributor

Lord-KA commented Nov 30, 2023

Personally, I don't see anything wrong with using yaml.encode() in this reproducer, but sure.

@igormunkin
Copy link
Collaborator

Also backported to 2.11 in scope of 074fe0b. To apply the changes for 2.10, @Buristan will create a separate PR with the corresponding updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full-ci Enables all tests for a pull request lua serializers Fearutes and bugs related to built-in serializers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants