Skip to content

Fix RapidJSON null allocator deref in schema validator#167

Merged
SergioRZMasson merged 3 commits into
microsoft:Release/1.9.5from
SergioRZMasson:rapidjson-fix-null-allocator-deref-1.9.5
May 5, 2026
Merged

Fix RapidJSON null allocator deref in schema validator#167
SergioRZMasson merged 3 commits into
microsoft:Release/1.9.5from
SergioRZMasson:rapidjson-fix-null-allocator-deref-1.9.5

Conversation

@SergioRZMasson
Copy link
Copy Markdown
Contributor

Apply a small CMake patch script during the RapidJSON ExternalProject_Add download step to fix CWE-476: null pointer dereference in GenericSchemaValidator::EndMissingDependentProperties() (replaces &GetInvalidSchemaPointer().GetAllocator() with &GetStateAllocator()).

Adds regression tests for the missing-dependent-property crash and a Linux Clang ASAN/UBSAN sanitizer CI workflow.

Apply a small CMake patch script during the RapidJSON ExternalProject_Add
download step to fix CWE-476: null pointer dereference in
GenericSchemaValidator::EndMissingDependentProperties() (replaces
&GetInvalidSchemaPointer().GetAllocator() with &GetStateAllocator()).

Adds regression tests for the missing-dependent-property crash and a
Linux Clang ASAN/UBSAN sanitizer CI workflow.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR mitigates a RapidJSON schema validator null-pointer dereference (CWE-476) by applying a small CMake-based patch during RapidJSON’s ExternalProject_Add download step, and adds regression coverage plus a Linux Clang ASAN/UBSAN CI job to catch similar issues.

Changes:

  • Add two regression tests for missing-dependent-property schema validation failures.
  • Introduce a CMake patch script to modify RapidJSON’s schema.h during external download.
  • Add a reusable GitHub Actions sanitizer workflow and wire it into the main CI workflow.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
GLTFSDK.Test/Source/DeserializeTests.cpp Adds malformed JSON inputs and regression tests expecting ValidationException instead of a crash.
External/RapidJSON/patches/fix-null-allocator-deref.cmake Adds a CMake script to patch RapidJSON schema.h to avoid null allocator deref.
External/RapidJSON/CMakeRapidJSONDownload.txt.in Runs the patch script via PATCH_COMMAND during RapidJSON ExternalProject_Add.
.github/workflows/sanitizer.yml Adds a workflow to build with Clang + ASAN/UBSAN and run unit tests on Linux.
.github/workflows/ci.yml Adds a sanitizer job that calls the new sanitizer workflow.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread GLTFSDK.Test/Source/DeserializeTests.cpp
Comment thread External/RapidJSON/patches/fix-null-allocator-deref.cmake
Comment thread External/RapidJSON/CMakeRapidJSONDownload.txt.in
Comment thread GLTFSDK.Test/Source/DeserializeTests.cpp
RAND_MAX expands to 2147483647 which cannot be exactly represented as a
float. Cast RAND_MAX explicitly to float to silence the warning under
clang with -Werror (used by the sanitizer CI job).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bghgary

This comment was marked as outdated.

bghgary
bghgary previously requested changes May 4, 2026
Copy link
Copy Markdown
Contributor

@bghgary bghgary left a comment

Choose a reason for hiding this comment

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

[Reviewed by Copilot on behalf of @bghgary]

The fix is correct and the sanitizer CI is a nice add. Agree with all four of Copilot's inline comments.

The biggest one is #2string(FIND ... -1) returns STATUS instead of FATAL_ERROR when neither pattern is present. Combined with the GIT_TAG pin, a future RapidJSON bump can silently regress the security fix. Three states should be distinguished:

  • &GetStateAllocator() already present → already patched, return STATUS.
  • &GetInvalidSchemaPointer().GetAllocator() present → apply patch.
  • Neither → FATAL_ERROR.

#4c_missingDependentPropertyBufferView duplicates the existing c_invalidAccessorDependency; reuse it.

One nit beyond Copilot's comments: ASAN_OPTIONS: detect_leaks=0 disables leak detection. Intentional or just template default? Worth flipping to 1 if there's no reason to skip.

Approve once #2 and #4 are addressed.

@bghgary bghgary dismissed their stale review May 4, 2026 20:29

Switching verdict to approve

bghgary

This comment was marked as spam.

static_cast<T>(round(f*scale)) without clamping is undefined behavior when f falls outside the destination integer type's normalized range. UBSan flagged this on Linux (FloatToComponent<uint8_t> with a negative float), and clang on macOS produced values inconsistent with MSVC, causing AnimationUtils_Test_GetRotations to fail there while passing on Windows.

Clamp to [-1,1] for signed and [0,1] for unsigned types before scaling, matching the glTF 2.0 spec for normalized component encoding and the existing clamp in ComponentToFloat for signed types.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@SergioRZMasson SergioRZMasson merged commit cfe7606 into microsoft:Release/1.9.5 May 5, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants