Skip to content

Commit

Permalink
fix: JsonStringUtil attempts to serialize bad char* in error message (#…
Browse files Browse the repository at this point in the history
…12027)

Summary:
Pull Request resolved: #12027

JsonStringUtil uses VELOX_USER_CHECK_NE/VELOX_USER_CHECK_LE to compare char* to make sure one is
not at or beyond the end of the string.  The way the macro works, if it fails, it will attempt to serialize the char*
in the error message.  It doesn't not serialize the address but rather a string starting from the pointer (since it's
a char*).  Given this check has already determined the pointer is at or beyond the end of the string this
produces results that are useless at best.

Using VELOX_USER_CHECK avoids this serialization.

Caught via ASAN with expression fuzzer tests.

Reviewed By: kgpai

Differential Revision: D67875678

fbshipit-source-id: a383aca300a5c2fe34f6d4169bfe0a5b98aff0bc
  • Loading branch information
Kevin Wilfong authored and facebook-github-bot committed Jan 6, 2025
1 parent b926bdf commit b92e4bd
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 8 deletions.
16 changes: 8 additions & 8 deletions velox/functions/prestosql/json/JsonStringUtil.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -334,17 +334,17 @@ size_t normalizeForJsonParse(const char* input, size_t length, char* output) {
while (start < end) {
// Unescape characters that are escaped by \ character.
if (FOLLY_UNLIKELY(*start == '\\')) {
VELOX_USER_CHECK_NE(
start + 1, end, "Invalid escape sequence at the end of string");
VELOX_USER_CHECK(
start + 1 != end, "Invalid escape sequence at the end of string");
// Presto java implementation only unescapes the / character.
switch (*(start + 1)) {
case '/':
*pos++ = '/';
start += 2;
continue;
case 'u': {
VELOX_USER_CHECK_LE(
start + 5, end, "Invalid escape sequence at the end of string");
VELOX_USER_CHECK(
start + 5 <= end, "Invalid escape sequence at the end of string");

// Read 4 hex digits.
auto codePoint = parseHex(std::string_view(start + 2, 4));
Expand Down Expand Up @@ -428,16 +428,16 @@ size_t normalizedSizeForJsonParse(const char* input, size_t length) {
size_t outSize = 0;
while (start < end) {
if (FOLLY_UNLIKELY(*start == '\\')) {
VELOX_USER_CHECK_NE(
start + 1, end, "Invalid escape sequence at the end of string");
VELOX_USER_CHECK(
start + 1 != end, "Invalid escape sequence at the end of string");
switch (*(start + 1)) {
case '/':
++outSize;
start += 2;
continue;
case 'u': {
VELOX_USER_CHECK_LE(
start + 5, end, "Invalid escape sequence at the end of string");
VELOX_USER_CHECK(
start + 5 <= end, "Invalid escape sequence at the end of string");
auto codePoint = parseHex(std::string_view(start + 2, 4));
if (isHighSurrogate(codePoint) || isLowSurrogate(codePoint) ||
isSpecialCode(codePoint)) {
Expand Down
6 changes: 6 additions & 0 deletions velox/functions/prestosql/tests/JsonFunctionsTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,12 @@ TEST_F(JsonFunctionsTest, jsonParse) {
} catch (const VeloxUserError& e) {
ASSERT_EQ(e.context(), "Top-level Expression: json_parse(c0)");
}

// Test partial escape sequences.
VELOX_ASSERT_USER_THROW(
jsonParse("{\"k1\\"), "Invalid escape sequence at the end of string");
VELOX_ASSERT_USER_THROW(
jsonParse("{\"k1\\u"), "Invalid escape sequence at the end of string");
}

TEST_F(JsonFunctionsTest, canonicalization) {
Expand Down

0 comments on commit b92e4bd

Please sign in to comment.