Add null pointer check in XNNWeightsCache::look_up_or_insert #16242
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/16242
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 11d2980 with merge base c493e2d ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
Hi @abdelaziz-mahdy! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
This PR needs a
|
There was a problem hiding this comment.
Pull request overview
This PR adds a null pointer check in XNNWeightsCache::look_up_or_insert to prevent a SIGSEGV crash when comparing cached weight data. The fix validates that both the input pointer and the cached pointer are non-null before calling memcmp, returning SIZE_MAX (cache miss) when either is null.
Key Changes:
- Added null pointer validation for
ptrandsaved_ptrparameters before memory comparison - Returns SIZE_MAX when null pointers are detected, allowing normal cache insertion to proceed
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (ptr == nullptr || saved_ptr == nullptr) { | ||
| // If either pointer is null, cache is invalid | ||
| return SIZE_MAX; | ||
| } |
There was a problem hiding this comment.
The null pointer check for saved_ptr is a defensive fix but may indicate a data consistency issue. When saved_ptr is null, it means delete_packed_data set packed_data_ptrs_[offset] = nullptr (line 118), but the corresponding entry in name_to_packed_data_metadata_ still exists (it's only removed when ref_count reaches 0). This allows look_up to return a valid offset for deleted data.
Returning SIZE_MAX here triggers cache reinsertion, which could lead to accumulating stale entries in name_to_packed_data_metadata_ over multiple delete/recreate cycles. Consider whether look_up should validate that the pointer is non-null before returning the offset, or if the metadata cleanup in delete_packed_data should be more comprehensive.
| // Check for null pointers before calling memcmp | ||
| if (ptr == nullptr || saved_ptr == nullptr) { | ||
| // If either pointer is null, cache is invalid | ||
| return SIZE_MAX; | ||
| } |
There was a problem hiding this comment.
This fix adds null pointer validation for a crash scenario described in the PR, but there are no test cases covering this scenario. Consider adding a test that validates the behavior when either ptr or saved_ptr is null. For example, a test could simulate the case where delete_packed_data is called and then look_up_or_insert is called with the same cache key, verifying that it returns SIZE_MAX without crashing.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (ptr == nullptr || saved_ptr == nullptr) { | ||
| // If either pointer is null (due to deleted data or allocation failure), treat as cache miss | ||
| // and return SIZE_MAX to trigger reinsertion. | ||
| return SIZE_MAX; | ||
| } |
There was a problem hiding this comment.
The null pointer check prevents an immediate crash, but introduces a potential issue when ptr is null. If ptr is null and SIZE_MAX is returned, the function continues to the insertion path (lines 224-252) where ptr will be stored in packed_data_ptrs_ at line 250. This means a null pointer gets cached, which could cause the same crash later when another call to look_up_or_insert retrieves it via offset_to_addr.
Consider handling the two null scenarios differently:
- If
saved_ptris null (cache corruption/deletion), returning SIZE_MAX to re-insert is correct - If
ptris null (XNNPACK packing failure), the function should either return an error/special value that doesn't trigger insertion, or validateptris non-null before insertion at line 250
A safer approach would be to check if ptr is null before attempting insertion and handle it as an error case, while only checking saved_ptr for null at line 212.
| // Check for null pointers before calling memcmp | ||
| if (ptr == nullptr || saved_ptr == nullptr) { | ||
| // If either pointer is null (due to deleted data or allocation failure), treat as cache miss | ||
| // and return SIZE_MAX to trigger reinsertion. | ||
| return SIZE_MAX; | ||
| } |
There was a problem hiding this comment.
The fix addresses a critical crash scenario but lacks test coverage. Consider adding a test case that verifies the null pointer handling, particularly:
- A test that simulates the scenario where
delete_packed_datasets a pointer to null, thenlook_up_or_insertis called with the same cache key - Verification that the function returns SIZE_MAX and doesn't crash when encountering null pointers
- Validation that the cache remains in a consistent state after handling null pointers
This would help prevent regressions and validate the fix works as intended for the crash scenario described in issue #16241.
meta-cla
bot
added
the
CLA Signed
|
cc: @GregoryComer since this is related to the XNNPACK backend |
2 participants
Summary
This PR fixes a critical null pointer dereference crash (SIGSEGV) in the XNNPACK weights cache implementation that occurs when
look_up_or_insertattempts to compare cached data with null pointers.Problem
The crash occurs in
XNNWeightsCache::look_up_or_insertat line 211 (original) whenmemcmpis called without validating that the input pointers are non-null. This can happen in the following scenarios:saved_ptris null because packed data was previously deleted (set to nullptr at line 118 indelete_packed_data)ptrparameter to the functionThe crash manifests as:
Solution
Added a null pointer check before calling
memcmpto validate bothptrandsaved_ptr. When either pointer is null, the function returnsSIZE_MAXto indicate the cache entry is invalid, allowing the normal insertion path to proceed instead of crashing.Fixes #16241
Test plan
I Was not able to test the changes due to not having enough knowledge to integrate the direct build into android build.
Release Notes
Category: Release notes: backends
Fixed critical null pointer dereference crash in XNNPACK weights cache when comparing cached data. The fix adds null pointer validation before memory comparison, preventing SIGSEGV crashes during model loading and runtime creation.