Skip to content

reject out-of-range array index in const json_pointer access - #5327

Open
Angadi56 wants to merge 1 commit into
nlohmann:developfrom
Angadi56:json-pointer-const-bounds-check
Open

reject out-of-range array index in const json_pointer access#5327
Angadi56 wants to merge 1 commit into
nlohmann:developfrom
Angadi56:json-pointer-const-bounds-check

Conversation

@Angadi56

Copy link
Copy Markdown
Contributor

The const operator[](json_pointer) overload resolves an array reference token with ptr->operator[](array_index(token)), which lands on the const operator[](size_type), a bare std::vector::operator[] with no range check. array_index() accepts any decimal in 0 .. SIZE_MAX-1, so on {"a":[1,2,3]} a pointer like /a/5 computes array->data() + idx * sizeof(basic_json) and hands back a const reference built from whatever heap bytes follow the array. AddressSanitizer reports a heap-buffer-overflow read there. Since the type byte and the json_value union are then read from unrelated memory, a type byte that happens to land on object/array/string/binary makes the following 8 bytes get used as a pointer and dereferenced.

The check belongs in get_unchecked() rather than at the call site: a caller passes a string, so it cannot tell whether a token is in range without resolving the pointer itself. The same function already throws out_of_range.402 for -, which is likewise an index past the end, and get_checked() a few lines below performs exactly the idx >= size() test that was missing here, so this mirrors it and reuses its 401 message. That makes j["/a/5"_json_pointer] report what j.at("/a/5"_json_pointer) already reports for the same pointer. In-range pointers resolve unchanged, and the non-const overload keeps filling the array with nulls as documented.

Verified with the full test suite (109/109) plus make check-amalgamation and cpplint. The new checks in unit-json_pointer.cpp fail on develop and pass here.

  • The changes are described in detail, both the what and why.
  • If applicable, an existing issue is referenced.
  • The Code coverage remained at 100%. A test case for every new line of code.
  • If applicable, the documentation is updated.
  • The source code is amalgamated by running make amalgamate.

Read the Contribution Guidelines for detailed information.

Signed-off-by: Angadi Yashaswini <angadi@digiscrypt.com>

@nlohmann nlohmann left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

But operator[] is not supposed to do range checks - this is what at() is for. Or what am I missing?

@Angadi56

Copy link
Copy Markdown
Contributor Author

You're right on the general contract, and I'd agree without argument if this were operator[](size_type).

What made me file it is that the const json_pointer overload isn't actually unchecked today. A few lines above this hunk the same function throws out_of_range.402 for -, which is also just an index past the end. So on {"a":[1,2,3]}, /a/- throws while /a/5 reads off the end of the array and returns a reference built from whatever heap bytes follow it. ASan reports it as a heap-buffer-overflow read, and since the type byte comes from unrelated memory too, a byte that happens to land on object/array/string makes the next 8 bytes get used as a pointer.

It also differs from operator[](size_type) in that the caller can't screen for it: with an index you can test idx < size() first, but with a pointer you only have a string, so there's no way to know a token is in range without resolving it yourself. And the UB block in operator[].md currently lists only overloads 1 and 2, not the pointer one, where the const object form at least has a JSON_ASSERT behind it.

Where the line sits is your call though. If you'd rather keep operator[] uniformly unchecked, I'll drop the throw and either mirror the const object overload with a JSON_ASSERT, or just document the UB for the pointer form. Tell me which you prefer and I'll rework it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants