Skip to content

Commit 368c912

Browse files
Daniel Huntefacebook-github-bot
Daniel Hunte
authored andcommitted
Modify pad function to handle invalid unicode string for padString (facebookincubator#10549)
Summary: Pull Request resolved: facebookincubator#10549 Fuzzer caught this bug: https://www.internalfb.com/chronos/job_instance/silver/3377708311833077/simple-logs Reviewed By: kevinwilfong Differential Revision: D60181544 fbshipit-source-id: 2b8857db6d2217db10e772a8d5f7ae887450b1a3
1 parent db11b69 commit 368c912

File tree

2 files changed

+17
-5
lines changed

2 files changed

+17
-5
lines changed

velox/functions/lib/string/StringImpl.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -593,6 +593,8 @@ FOLLY_ALWAYS_INLINE void pad(
593593
"pad size must be in the range [0..{})",
594594
padMaxSize);
595595
VELOX_USER_CHECK(padString.size() > 0, "padString must not be empty");
596+
int64_t padStringCharLength = length<isAscii>(padString);
597+
VELOX_USER_CHECK(padStringCharLength > 0, "padString must be a valid string");
596598

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

611-
int64_t padStringCharLength = length<isAscii>(padString);
612613
// How many characters do we need to add to string.
613614
int64_t fullPaddingCharLength = size - stringCharLength;
614615
// How many full copies of padString need to be added.

velox/functions/lib/string/tests/StringImplTest.cpp

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -652,10 +652,18 @@ TEST_F(StringImplTest, pad) {
652652
const std::string& padString) {
653653
core::StringWriter output;
654654

655-
EXPECT_THROW(
656-
(facebook::velox::functions::stringImpl::pad<true, true>(
657-
output, StringView(string), size, StringView(padString))),
658-
VeloxUserError);
655+
bool padStringIsAscii = isAscii(padString.c_str(), padString.size());
656+
if (padStringIsAscii) {
657+
EXPECT_THROW(
658+
(facebook::velox::functions::stringImpl::pad<true, true>(
659+
output, StringView(string), size, StringView(padString))),
660+
VeloxUserError);
661+
} else {
662+
EXPECT_THROW(
663+
(facebook::velox::functions::stringImpl::pad<true, false>(
664+
output, StringView(string), size, StringView(padString))),
665+
VeloxUserError);
666+
}
659667
};
660668

661669
// ASCII string with various values for size and padString
@@ -712,6 +720,9 @@ TEST_F(StringImplTest, pad) {
712720
runTest(
713721
"abcd\xff \xff ef", 11, "0", "0abcd\xff \xff ef", "abcd\xff \xff ef0");
714722
runTest("abcd\xff ef", 6, "0", "abcd\xff ", "abcd\xff ");
723+
// Testcase for when padString is a sequence of unicode continuation bytes
724+
// for which effective length is 0.
725+
runTestUserError(/*string=*/"\u4FE1", /*size=*/6, /*padString=*/"\xBF\xBF");
715726
}
716727

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

0 commit comments

Comments
 (0)