Skip to content

Add null pointer check in XNNWeightsCache::look_up_or_insert #16242

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
abdelaziz-mahdy wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add null pointer check in XNNWeightsCache::look_up_or_insert #16242

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e1fb1f9
Add null pointer check in XNNWeightsCache::look_up_or_insert
abdelaziz-mahdy Dec 14, 2025
11d2980
Update backends/xnnpack/runtime/XNNWeightsCache.cpp
abdelaziz-mahdy Dec 15, 2025
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
6 changes: 6 additions & 0 deletions backends/xnnpack/runtime/XNNWeightsCache.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,12 @@ size_t XNNWeightsCache::look_up_or_insert(

if (offset != SIZE_MAX) {
void* saved_ptr = context->offset_to_addr(context, offset);
// 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;
}
Comment on lines +212 to +216
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

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

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_ptr is null (cache corruption/deletion), returning SIZE_MAX to re-insert is correct
  • If ptr is null (XNNPACK packing failure), the function should either return an error/special value that doesn't trigger insertion, or validate ptr is 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.

Copilot uses AI. Check for mistakes.
Comment on lines +211 to +216
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

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

The fix addresses a critical crash scenario but lacks test coverage. Consider adding a test case that verifies the null pointer handling, particularly:

  1. A test that simulates the scenario where delete_packed_data sets a pointer to null, then look_up_or_insert is called with the same cache key
  2. Verification that the function returns SIZE_MAX and doesn't crash when encountering null pointers
  3. 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.

Copilot uses AI. Check for mistakes.
if (0 == memcmp(ptr, saved_ptr, size)) {
return offset;
}
Expand Down
Loading