Skip to content

Reset the trace queue indexes when the queue shrinks - #1694

Draft
AbhinavMir wants to merge 1 commit into
eclipse-paho:developfrom
AbhinavMir:trace-queue-bounds
Draft

Reset the trace queue indexes when the queue shrinks#1694
AbhinavMir wants to merge 1 commit into
eclipse-paho:developfrom
AbhinavMir:trace-queue-bounds

Conversation

@AbhinavMir

Copy link
Copy Markdown

Problem

Log_pretrace() reallocates the trace queue when an application changes trace_settings.max_trace_entries. The queue holds max_trace_entries entries, so the valid indexes are 0 to max_trace_entries - 1.

The bounds check compares the stored ring indexes with max_trace_entries + 1. It therefore accepts max_trace_entries and max_trace_entries + 1. Both are past the end of the new queue.

A shrink to the current write index leaves next_index out of range. Log_trace() and Log_stackTrace() then write a whole traceEntry past the end of the allocation. The index also never wraps again, because the wrap test uses ==. Every later entry goes further past the end.

Change

Compare the indexes with max_trace_entries.

Test

test/test_unit_coverage.c gets a new case. It uses 5 slots of a 10 entry queue, shrinks the queue to 5 entries, and keeps logging. A trace callback counts the entries.

Without the fix:

  • AddressSanitizer reports a heap buffer overflow in Log_trace(), 0 bytes to the right of the 1560 byte queue.
  • A plain build aborts with double free or corruption (!prev).

With the fix the test passes in both builds.

Build

Built with gcc 12 on Debian 12. Static and shared, with OpenSSL, plus an AddressSanitizer build. No new compiler warnings.

test1 cases 1 to 5 and test4 cases 1 to 3 pass against a local broker.

The commit is signed off.

Log_pretrace() reallocates the trace queue when an application changes
trace_settings.max_trace_entries. The valid indexes are 0 to
max_trace_entries - 1. The bounds check compared the stored ring indexes
with max_trace_entries + 1, so it accepted two indexes that are past the
end of the new queue.

A shrink to the current write index therefore left the index out of
range. The caller then wrote a trace entry past the end of the
allocation. The index also never wrapped again, so every later entry
went further past the end.

Compare the indexes with max_trace_entries instead.

Add a unit test that shrinks the queue to the current write index.
AddressSanitizer reports a heap buffer overflow in Log_trace() without
this fix, and glibc aborts the plain build with heap corruption.

Signed-off-by: Abhinav Srivastava <atg271@gmail.com>
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.

1 participant