Copilot commented on code in PR #13170:
URL: https://github.com/apache/trafficserver/pull/13170#discussion_r3356120274
##########
ci/asan_leak_suppression/unit_tests.txt:
##########
@@ -2,3 +2,8 @@
# not destroyed because it holds a reference to a stack-allocated
# TestRefCountObj whose free() override calls exit(1).
leak:test_http_hdr_print_and_copy_aux
+
+# on 64-bit architectures, freelist pointers are stored in a special bitwise
+# format, so LSan cannot find link pointers. It will erroneously determine
+# that the list tail is not reachable.
+leak:freelist_new
Review Comment:
Adding an LSan suppression for `freelist_new` will suppress *all* leaks
whose allocation stack includes freelist chunk allocation, which significantly
reduces the usefulness of leak checking in unit tests (real leaks from
forgotten `ink_freelist_free()` calls will also be hidden). Since this was
introduced to work around pointer-encoding reachability issues, consider an
ASan/LSan-specific mechanism to keep freelist chunks reachable (e.g. store a
raw head pointer or chunk list under `__has_feature(address_sanitizer)` /
`__SANITIZE_ADDRESS__`) rather than globally suppressing the allocator function.
##########
src/tscore/ink_queue.cc:
##########
@@ -137,13 +144,32 @@ void
ink_freelist_init(InkFreeList **fl, const char *name, uint32_t type_size,
uint32_t chunk_size, uint32_t alignment,
bool use_hugepages)
{
+ // The alignment is used as a boundary for INK_ALIGN,
+ // which requires a power of 2 boundary.
+ ink_release_assert(alignment != 0 && (alignment & (alignment - 1u)) == 0);
+
+ // The same alignment is used for both the InkFreeList object and to
+ // calculate the alignment member that determines alignment for
+ // chunks allocated in ink_freelist_new. I don't know why.
+ //
+ // The alignment may need to be corrected to ensure all internal bookkeeping
+ // objects are properly aligned, not only the user objects placed into the
+ // allocated memory.
+ alignment = std::lcm(alignment,
static_cast<std::uint32_t>(alignof(InkFreeList)));
+ alignment = std::lcm(alignment, static_cast<std::uint32_t>(alignof(void *)));
Review Comment:
The new comment explains the alignment reuse but includes “I don't know
why.”, which is not actionable and makes the rationale unclear for future
maintainers. It would be better to state the concrete reason for the adjustment
(ensuring the freelist header and internal bookkeeping are correctly aligned)
without speculation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
