Skip to content

Commit

Permalink
Modify pad function to handle invalid unicode string for padString
Browse files Browse the repository at this point in the history
Summary:
Fuzzer caught this bug:

https://www.internalfb.com/chronos/job_instance/silver/3377708311833077/simple-logs

Differential Revision: D60181544
  • Loading branch information
Daniel Hunte authored and facebook-github-bot committed Jul 24, 2024
1 parent 40122c6 commit 22630ac
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 5 deletions.
3 changes: 2 additions & 1 deletion velox/functions/lib/string/StringImpl.h
Original file line number Diff line number Diff line change
Expand Up @@ -593,6 +593,8 @@ FOLLY_ALWAYS_INLINE void pad(
"pad size must be in the range [0..{})",
padMaxSize);
VELOX_USER_CHECK(padString.size() > 0, "padString must not be empty");
int64_t padStringCharLength = length<isAscii>(padString);
VELOX_USER_CHECK(padStringCharLength > 0, "padString must be a valid string");

int64_t stringCharLength = length<isAscii>(string);
// If string has at most size characters, truncate it if necessary
Expand All @@ -608,7 +610,6 @@ FOLLY_ALWAYS_INLINE void pad(
return;
}

int64_t padStringCharLength = length<isAscii>(padString);
// How many characters do we need to add to string.
int64_t fullPaddingCharLength = size - stringCharLength;
// How many full copies of padString need to be added.
Expand Down
17 changes: 13 additions & 4 deletions velox/functions/lib/string/tests/StringImplTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -652,10 +652,18 @@ TEST_F(StringImplTest, pad) {
const std::string& padString) {
core::StringWriter output;

EXPECT_THROW(
(facebook::velox::functions::stringImpl::pad<true, true>(
output, StringView(string), size, StringView(padString))),
VeloxUserError);
bool padStringIsAscii = isAscii(padString.c_str(), padString.size());
if (padStringIsAscii) {
EXPECT_THROW(
(facebook::velox::functions::stringImpl::pad<true, true>(
output, StringView(string), size, StringView(padString))),
VeloxUserError);
} else {
EXPECT_THROW(
(facebook::velox::functions::stringImpl::pad<true, false>(
output, StringView(string), size, StringView(padString))),
VeloxUserError);
}
};

// ASCII string with various values for size and padString
Expand Down Expand Up @@ -712,6 +720,7 @@ TEST_F(StringImplTest, pad) {
runTest(
"abcd\xff \xff ef", 11, "0", "0abcd\xff \xff ef", "abcd\xff \xff ef0");
runTest("abcd\xff ef", 6, "0", "abcd\xff ", "abcd\xff ");
runTestUserError(/*string=*/"\u4FE1", /*size=*/6, /*padString=*/"\xBF\xBF");
}

// Make sure that utf8proc_codepoint returns invalid codepoint (-1) for
Expand Down

0 comments on commit 22630ac

Please sign in to comment.