Skip to content

ADBM-3574: Fix JSON support for pgbackrest recovery filters #113

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

Open
VoidZeroNull0 wants to merge 5 commits into
base: 2.54-ci
Choose a base branch
from
+92 −17
Open

ADBM-3574: Fix JSON support for pgbackrest recovery filters #113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
de82c8a
Initial patch
VoidZeroNull0 Oct 28, 2026
83aafa3
Remove test for unknown encoding
VoidZeroNull0 Oct 29, 2026
1fbd4bb
Fix increment bug
VoidZeroNull0 Oct 29, 2026
c8b48a4
Fix increment bug again
VoidZeroNull0 Oct 29, 2026
5f32798
Refactor and add tests
VoidZeroNull0 Oct 29, 2026
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
70 changes: 57 additions & 13 deletions src/common/type/json.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -819,12 +819,12 @@ jsonReadSkipStr(JsonRead *const this)
{
this->json++;

// Expect four digits
// Expect four hex digits
unsigned int digitIdx = 0;

for (; digitIdx < 4; digitIdx++)
{
if (!isdigit(this->json[digitIdx]))
if (!isxdigit(this->json[digitIdx]))

@RekGRpth RekGRpth Oct 30, 2026

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.

A \ before \0 in jsonReadSkipStr causes the pointer to move past the end of the string (fixed upstream in pgbackrest/pgbackrest@b623071a2).

break;
}

Expand Down Expand Up @@ -937,6 +937,60 @@ jsonReadSkip(JsonRead *const this)
FUNCTION_TEST_RETURN_VOID();
}

/**********************************************************************************************************************************/
FN_EXTERN void
jsonDecodeUnicode(JsonRead *const this, String *const result)
{
this->json++; // get past /u
Comment on lines +941 to +944

@RekGRpth RekGRpth Oct 30, 2026

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.

add FUNCTION_TEST_BEGIN/PARAM/RETURN and ASSERT which CODING.md requires

const unsigned int high = cvtZSubNToUIntBase(this->json, 0, 4, 16);
unsigned int codepoint = high;
this->json += 4;

// Check for supplementary planes
if (high >= 0xD800 && high <= 0xDBFF)
{
if (*this->json != '\\' || *(this->json + 1) != 'u')
THROW_FMT(JsonFormatError, "missing low surrogate for high surrogate at: %s", this->json - 6);

this->json += 2;
const unsigned int low = cvtZSubNToUIntBase(this->json, 0, 4, 16);
this->json += 4;

if (low < 0xDC00 || low > 0xDFFF)
THROW_FMT(JsonFormatError, "invalid low surrogate in unicode escape at: %s", this->json - 6);

codepoint = 0x10000 + ((high - 0xD800) << 10) + (low - 0xDC00);
}
else if (high >= 0xDC00 && high <= 0xDFFF)
THROW_FMT(JsonFormatError, "unexpected low surrogate without high surrogate at: %s", this->json - 4);

@RekGRpth RekGRpth Oct 30, 2026

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.

Suggested change
THROW_FMT(JsonFormatError, "unexpected low surrogate without high surrogate at: %s", this->json - 4);
THROW_FMT(JsonFormatError, "unexpected low surrogate without high surrogate at: %s", this->json - 6);

to add \u in error message like above


// Append 1-4 bytes to the result
if (codepoint <= 0x7F)
{
strCatChr(result, (char)codepoint);
}
else if (codepoint <= 0x7FF)
{
strCatChr(result, (char)(0xC0 | (codepoint >> 6)));
strCatChr(result, (char)(0x80 | (codepoint & 0x3F)));
}
else if (codepoint <= 0xFFFF)
{
strCatChr(result, (char)(0xE0 | (codepoint >> 12)));
strCatChr(result, (char)(0x80 | ((codepoint >> 6) & 0x3F)));
strCatChr(result, (char)(0x80 | (codepoint & 0x3F)));
}
else
{
strCatChr(result, (char)(0xF0 | (codepoint >> 18)));
strCatChr(result, (char)(0x80 | ((codepoint >> 12) & 0x3F)));
strCatChr(result, (char)(0x80 | ((codepoint >> 6) & 0x3F)));
strCatChr(result, (char)(0x80 | (codepoint & 0x3F)));
}
Comment on lines +972 to +989

@RekGRpth RekGRpth Oct 30, 2026

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.

these lines are not covered by tests

@RekGRpth RekGRpth Oct 30, 2026

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.

This is precisely why the CI is red: it checks the coverage of the ENTIRE codebase.


this->json--;
}

Comment on lines +940 to +993

@RekGRpth RekGRpth Oct 30, 2026

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.

jsonDecodeUnicode() lacks validation for 4 hex digits (it reads past the \0 terminator, throws FormatError instead of JsonFormatError, and strtoull accepts extraneous input).

/**********************************************************************************************************************************/
FN_EXTERN String *
jsonReadStr(JsonRead *const this)
Expand Down Expand Up @@ -1016,17 +1070,7 @@ jsonReadStr(JsonRead *const this)

case 'u':
{
this->json++;

// We don't know how to decode anything except ASCII so fail if it looks like Unicode
if (strncmp(this->json, "00", 2) != 0)
THROW_FMT(JsonFormatError, "unable to decode at: %s", this->json - 2);

// Decode char
this->json += 2;
strCatChr(result, (char)cvtZSubNToUIntBase(this->json, 0, 2, 16));
this->json++;

jsonDecodeUnicode(this, result);

@RekGRpth RekGRpth Oct 30, 2026

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.

Now jsonReadStr does not validate the input data, and jsonDecodeUnicode does not check it either.

break;
}

Expand Down
3 changes: 3 additions & 0 deletions src/common/type/json.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,9 @@ FN_EXTERN Variant *jsonReadVar(JsonRead *this);
/***********************************************************************************************************************************
Read Helper Functions
***********************************************************************************************************************************/
// Unicode escape
FN_EXTERN void jsonDecodeUnicode(JsonRead *this, String *result);

// Convert JSON to Variant

@RekGRpth RekGRpth Oct 30, 2026

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.

Why did you make this function public?

FN_EXTERN Variant *jsonToVar(const String *json);

Expand Down
36 changes: 32 additions & 4 deletions test/src/module/common/typeJsonTest.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,14 +57,42 @@ testRun(void)
JsonFormatError, "invalid number at: 9999999999999999999999999999999999999999999999999999999999999999");

// -------------------------------------------------------------------------------------------------------------------------
TEST_TITLE("error on invalid ascii encoding");
TEST_TITLE("error on invalid escape");

TEST_ERROR(jsonReadStr(jsonReadNew(STRDEF("\"\\u1111"))), JsonFormatError, "unable to decode at: \\u1111");
TEST_ERROR(jsonReadStr(jsonReadNew(STRDEF("\"\\xy"))), JsonFormatError, "invalid escape character at: \\xy");

// -------------------------------------------------------------------------------------------------------------------------
TEST_TITLE("error on invalid escape");
TEST_TITLE("unicode escape followed by escape");

TEST_ERROR(jsonReadStr(jsonReadNew(STRDEF("\"\\xy"))), JsonFormatError, "invalid escape character at: \\xy");
TEST_RESULT_STR_Z(jsonReadStr(jsonReadNew(STRDEF("\"\\u0076\\n\""))), "v\n", "unicode then newline");

// -------------------------------------------------------------------------------------------------------------------------
TEST_TITLE("error on high surrogate without low surrogate");

TEST_ERROR(
jsonReadStr(jsonReadNew(STRDEF("\"\\uD83D\""))), JsonFormatError,
"missing low surrogate for high surrogate at: \\uD83D\"");

// -------------------------------------------------------------------------------------------------------------------------
TEST_TITLE("error on high surrogate followed by non-unicode escape");

TEST_ERROR(
jsonReadStr(jsonReadNew(STRDEF("\"\\uD83D\\n\""))), JsonFormatError,
"missing low surrogate for high surrogate at: \\uD83D\\n\"");

// -------------------------------------------------------------------------------------------------------------------------
TEST_TITLE("error on invalid low surrogate");

TEST_ERROR(
jsonReadStr(jsonReadNew(STRDEF("\"\\uD83D\\u0041\""))), JsonFormatError,
"invalid low surrogate in unicode escape at: \\u0041\"");

// -------------------------------------------------------------------------------------------------------------------------
TEST_TITLE("error on low surrogate without high surrogate");

TEST_ERROR(
jsonReadStr(jsonReadNew(STRDEF("\"\\uDE00\""))), JsonFormatError,
"unexpected low surrogate without high surrogate at: DE00\"");

// -------------------------------------------------------------------------------------------------------------------------
TEST_TITLE("error on unterminated string");
Expand Down
Loading