Fix issue #4476 - #4477
Conversation
Signed-off-by: Allen Samuels <allenss@amazon.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthrough
ChangesHash lookup safety
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to The PR adds a localized null check with accompanying tests, and no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| if (objectGetEncoding(o) == OBJ_ENCODING_LISTPACK) return false; | ||
| hashtable *ht = objectGetVal(o); | ||
| void **entry_ref = hashtableFindRef(ht, field); | ||
| if (!entry_ref) return false; |
There was a problem hiding this comment.
This crash path still has no regression coverage: tests/unit/moduleapi/hash_stringref.tcl only queries the existing field f. After hash.set_stringref has converted k to a hashtable, add assert_equal "0" [r hash.has_stringref k missing]; that specifically exercises hashtableFindRef() returning NULL and protects this guard.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #4477 +/- ##
============================================
- Coverage 78.78% 78.71% -0.07%
============================================
Files 170 170
Lines 89792 89793 +1
============================================
- Hits 70742 70684 -58
- Misses 19050 19109 +59
🚀 New features to boost your workflow:
|
hashTypeHasStringRef() returns early for listpack-encoded hashes, so the NULL entry_ref deref is only reachable once the hash is converted to the hashtable encoding. The test sets a stringref to force that conversion, then queries a field that does not exist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FS8pBvk33tMU16JM5q1tB9 Signed-off-by: Allen Samuels <allenss@amazon.com>
Add a null check to avoid the crash.