Skip to content

lua: Fix printing Lua string containing hex characters #17994

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

Merged
mattklein123 merged 20 commits into from
Sep 15, 2021
Merged

lua: Fix printing Lua string containing hex characters #17994

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
35c1bb7
fix luaSetBytes as char
Patrick0308 Sep 6, 2021
ca56457
fix lua script log be truncated
Patrick0308 Sep 6, 2021
3eccad4
add a checkLuaStringWithLength function
Patrick0308 Sep 7, 2021
559ba90
format code and modify comment
Patrick0308 Sep 7, 2021
468c0c5
add test setBytes with empty string
Patrick0308 Sep 7, 2021
50dcaca
fix httpCall test failed
Patrick0308 Sep 8, 2021
141afac
add unit test for zero byte
Patrick0308 Sep 8, 2021
7785a54
modify comment and function name
Patrick0308 Sep 8, 2021
1362378
init input_size
Patrick0308 Sep 9, 2021
a492a31
add comment in test
Patrick0308 Sep 10, 2021
eb34c1c
fix
Patrick0308 Sep 10, 2021
c6e70f6
modify comment and add a test for unexpected type
Patrick0308 Sep 13, 2021
657db60
format
Patrick0308 Sep 13, 2021
90cdf26
fix spell
Patrick0308 Sep 13, 2021
badd509
add doc
Patrick0308 Sep 13, 2021
571966f
modify comment
Patrick0308 Sep 13, 2021
924c805
modify doc
Patrick0308 Sep 13, 2021
aa6ff26
modify doc
Patrick0308 Sep 13, 2021
d33d446
Merge remote-tracking branch 'upstream/main' into fix-luaSetBytes
Patrick0308 Sep 13, 2021
3af7a01
modify doc
Patrick0308 Sep 13, 2021
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: 13 additions & 0 deletions source/extensions/filters/common/lua/lua.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,19 @@ namespace Lua {
lua_pushnumber(state, val); \
lua_settable(state, -3);

/**
* Get absl::string_view from Lua string. This checks if the argument at index is a string
* and build an absl::string_view from it.
* @param state the current Lua state.
* @param index the index of argument.
* @return absl::string_view of Lua string with proper string length.
**/
inline absl::string_view getStringViewFromLuaString(lua_State* state, int arg) {
Comment thread
Patrick0308 marked this conversation as resolved.
Outdated
size_t input_size = 0;
const char* input = luaL_checklstring(state, arg, &input_size);
Comment thread
Patrick0308 marked this conversation as resolved.
Outdated

@wbpcode wbpcode Sep 10, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

According to @dio 's comments, we also need to add a comment here to explain the case of parameter errors (the case that luaL_checklstring cannot get valid string).

return absl::string_view(input, input_size);
}

/**
* Calculate the maximum space needed to be aligned.
*/
Expand Down
3 changes: 1 addition & 2 deletions source/extensions/filters/common/lua/wrappers.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ int64_t timestampInSeconds(const absl::optional<SystemTime>& system_time) {
.count()
: 0;
}

} // namespace

int BufferWrapper::luaLength(lua_State* state) {
Expand All @@ -67,7 +66,7 @@ int BufferWrapper::luaGetBytes(lua_State* state) {

int BufferWrapper::luaSetBytes(lua_State* state) {
data_.drain(data_.length());
absl::string_view bytes = luaL_checkstring(state, 2);
absl::string_view bytes = getStringViewFromLuaString(state, 2);
data_.add(bytes);
lua_pushnumber(state, data_.length());
return 1;
Expand Down
19 changes: 9 additions & 10 deletions source/extensions/filters/http/lua/lua_filter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -552,37 +552,37 @@ int StreamHandleWrapper::luaConnection(lua_State* state) {
}

int StreamHandleWrapper::luaLogTrace(lua_State* state) {
const char* message = luaL_checkstring(state, 2);
absl::string_view message = Filters::Common::Lua::getStringViewFromLuaString(state, 2);
filter_.scriptLog(spdlog::level::trace, message);
return 0;
}

int StreamHandleWrapper::luaLogDebug(lua_State* state) {
const char* message = luaL_checkstring(state, 2);
absl::string_view message = Filters::Common::Lua::getStringViewFromLuaString(state, 2);
filter_.scriptLog(spdlog::level::debug, message);
return 0;
}

int StreamHandleWrapper::luaLogInfo(lua_State* state) {
const char* message = luaL_checkstring(state, 2);
absl::string_view message = Filters::Common::Lua::getStringViewFromLuaString(state, 2);
filter_.scriptLog(spdlog::level::info, message);
return 0;
}

int StreamHandleWrapper::luaLogWarn(lua_State* state) {
const char* message = luaL_checkstring(state, 2);
absl::string_view message = Filters::Common::Lua::getStringViewFromLuaString(state, 2);
filter_.scriptLog(spdlog::level::warn, message);
return 0;
}

int StreamHandleWrapper::luaLogErr(lua_State* state) {
const char* message = luaL_checkstring(state, 2);
absl::string_view message = Filters::Common::Lua::getStringViewFromLuaString(state, 2);
filter_.scriptLog(spdlog::level::err, message);
return 0;
}

int StreamHandleWrapper::luaLogCritical(lua_State* state) {
const char* message = luaL_checkstring(state, 2);
absl::string_view message = Filters::Common::Lua::getStringViewFromLuaString(state, 2);
filter_.scriptLog(spdlog::level::critical, message);
return 0;
}
Expand Down Expand Up @@ -649,9 +649,8 @@ int StreamHandleWrapper::luaImportPublicKey(lua_State* state) {
}

int StreamHandleWrapper::luaBase64Escape(lua_State* state) {
size_t input_size;
const char* input = luaL_checklstring(state, 2, &input_size);
auto output = absl::Base64Escape(absl::string_view(input, input_size));
absl::string_view input = Filters::Common::Lua::getStringViewFromLuaString(state, 2);
auto output = absl::Base64Escape(input);
lua_pushlstring(state, output.data(), output.length());

return 1;
Expand Down Expand Up @@ -783,7 +782,7 @@ void Filter::scriptError(const Filters::Common::Lua::LuaException& e) {
response_stream_wrapper_.reset();
}

void Filter::scriptLog(spdlog::level::level_enum level, const char* message) {
void Filter::scriptLog(spdlog::level::level_enum level, absl::string_view message) {
switch (level) {
case spdlog::level::trace:
ENVOY_LOG(trace, "script log: {}", message);
Expand Down
2 changes: 1 addition & 1 deletion source/extensions/filters/http/lua/lua_filter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,7 @@ class Filter : public Http::StreamFilter, Logger::Loggable<Logger::Id::lua> {

Upstream::ClusterManager& clusterManager() { return config_->cluster_manager_; }
void scriptError(const Filters::Common::Lua::LuaException& e);
virtual void scriptLog(spdlog::level::level_enum level, const char* message);
virtual void scriptLog(spdlog::level::level_enum level, absl::string_view message);

// Http::StreamFilterBase
void onDestroy() override;
Expand Down
57 changes: 54 additions & 3 deletions test/extensions/filters/http/lua/lua_filter_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class TestFilter : public Filter {
public:
using Filter::Filter;

MOCK_METHOD(void, scriptLog, (spdlog::level::level_enum level, const char* message));
MOCK_METHOD(void, scriptLog, (spdlog::level::level_enum level, absl::string_view message));
};

class LuaHttpFilterTest : public testing::Test {
Expand Down Expand Up @@ -796,7 +796,7 @@ TEST_F(LuaHttpFilterTest, HttpCall) {
request_handle:logTrace(key .. " " .. value)
end
request_handle:logTrace(string.len(body))
request_handle:logTrace(body)
request_handle:logTrace(string.sub(body, 1, 4) .. string.byte(body, 5) .. string.sub(body, 6, 8))
request_handle:logTrace(string.byte(body, 5))
request_handle:logTrace(string.sub(body, 6, 8))
end
Expand All @@ -818,6 +818,7 @@ TEST_F(LuaHttpFilterTest, HttpCall) {
{":method", "POST"},
{":path", "/"},
{":authority", "foo"},

{"set-cookie", "flavor=chocolate; Path=/"},
{"set-cookie", "variant=chewy; Path=/"},
{"content-length", "11"}};
Expand All @@ -841,7 +842,7 @@ TEST_F(LuaHttpFilterTest, HttpCall) {
response_message->body().add(response, 8);
EXPECT_CALL(*filter_, scriptLog(spdlog::level::trace, StrEq(":status 200")));
EXPECT_CALL(*filter_, scriptLog(spdlog::level::trace, StrEq("8")));
EXPECT_CALL(*filter_, scriptLog(spdlog::level::trace, StrEq("resp")));
EXPECT_CALL(*filter_, scriptLog(spdlog::level::trace, StrEq("resp0nse")));
Comment thread
Patrick0308 marked this conversation as resolved.
Outdated
EXPECT_CALL(*filter_, scriptLog(spdlog::level::trace, StrEq("0")));
EXPECT_CALL(*filter_, scriptLog(spdlog::level::trace, StrEq("nse")));
EXPECT_CALL(decoder_callbacks_, continueDecoding());
Expand Down Expand Up @@ -2392,6 +2393,56 @@ TEST_F(LuaHttpFilterTest, LuaFilterSetResponseBufferChunked) {
EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->encodeData(response_body, true));
}

Comment thread
Patrick0308 marked this conversation as resolved.
// bodyBuffer should not truncated when bodyBuffer set hex character

@wbpcode wbpcode Sep 10, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

May be:

Test the case that updating the body buffer with a string with hex characters.

TEST_F(LuaHttpFilterTest, LuaBodyBufferSetBytesWithHex) {
Comment thread
Patrick0308 marked this conversation as resolved.
const std::string SCRIPT{R"EOF(
function envoy_on_response(response_handle)
local bodyBuffer = response_handle:body()
bodyBuffer:setBytes("\x471111")
local body_str = bodyBuffer:getBytes(0, bodyBuffer:length())
response_handle:logTrace(body_str)
end
)EOF"};

InSequence s;
setup(SCRIPT);

Http::TestRequestHeaderMapImpl request_headers{{":path", "/"}};
EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, true));

Http::TestResponseHeaderMapImpl response_headers{{":status", "200"}};
EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
filter_->encodeHeaders(response_headers, false));

Buffer::OwnedImpl response_body("");
EXPECT_CALL(*filter_, scriptLog(spdlog::level::trace, StrEq("G1111")));
EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->encodeData(response_body, true));
EXPECT_EQ(5, encoder_callbacks_.buffer_->length());
}

Comment thread
Patrick0308 marked this conversation as resolved.
// bodyBuffer should not truncated when bodyBuffer set zero

@wbpcode wbpcode Sep 10, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

May be:

Test the case that updating the body buffer with a string with embedded zero ('\0').

TEST_F(LuaHttpFilterTest, LuaBodyBufferSetBytesWithZero) {
const std::string SCRIPT{R"EOF(
function envoy_on_response(response_handle)
local bodyBuffer = response_handle:body()
bodyBuffer:setBytes("\0")
end
)EOF"};

InSequence s;
setup(SCRIPT);

Http::TestRequestHeaderMapImpl request_headers{{":path", "/"}};
EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, true));

Http::TestResponseHeaderMapImpl response_headers{{":status", "200"}};
EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
filter_->encodeHeaders(response_headers, false));

Buffer::OwnedImpl response_body("1111");
EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->encodeData(response_body, true));
EXPECT_EQ(1, encoder_callbacks_.buffer_->length());
}
} // namespace
} // namespace Lua
} // namespace HttpFilters
Expand Down