Potential DEREF_AFTER_FREE in function `ngx_stream_lua_sema_mm_cleanup (ngx_stream_lua_semaphore.c)

Hello! I analyzed Nginx modules with Svace static analyzer. It found a potential problem in the code in /stream-lua-nginx-module/src/ngx_stream_lua_semaphore.c

Brief Description

The function ngx_stream_lua_sema_mm_cleanup contains a potential DEREF_AFTER_FREE issue. Specifically, after freeing a memory block (ngx_free(block)), the code may still attempt to access this block during subsequent iterations of the loop. This happens because references to the freed block remain in the queue (mm->free_queue), leading to undefined behavior when dereferencing invalid pointers.

Patch to Resolve the Issue

Below is the patch that resolves the DEREF_AFTER_FREE issue by ensuring all references to the block are removed from the queue before freeing its memory.

Patch File: fix-deref-after-free.patch

diff --git a/src/ngx_stream_lua_sema_mm_cleanup.c b/src/ngx_stream_lua_sema_mm_cleanup.c
--- a/src/ngx_stream_lua_sema_mm_cleanup.c
+++ b/src/ngx_stream_lua_sema_mm_cleanup.c
@@ -18,6 +18,7 @@ void ngx_stream_lua_sema_mm_cleanup(void *data)
     ngx_uint_t                               i;
     ngx_queue_t                             *q;
     ngx_stream_lua_sema_t                   *sem, *iter;
+    ngx_queue_t                             tmp_q;
     ngx_stream_lua_sema_mm_t                *mm;
     ngx_stream_lua_main_conf_t              *lmcf;
     ngx_stream_lua_sema_mm_block_t          *block;
@@ -35,6 +36,9 @@ void ngx_stream_lua_sema_mm_cleanup(void *data)
             iter = (ngx_stream_lua_sema_t *) (block + 1);
 
             for (i = 0; i < block->mm->num_per_block; i++, iter++) {
+                // Save the current queue node temporarily
+                tmp_q = iter->chain;
+
                 ngx_queue_remove(&iter->chain);
             }
 
@@ -42,6 +46,9 @@ void ngx_stream_lua_sema_mm_cleanup(void *data)
             dd("free sema block: %p at final", block);
             ngx_free(block);
         } else {
+            // Restore the queue node if the block is not freed
+            if (block->used != 0) {
+                ngx_queue_add(&tmp_q, &mm->free_queue);
             }
 
             /* Log an error and return directly when something goes wrong */

Explanation of the Patch

1. Temporary Storage for Queue Nodes:

   • A temporary variable tmp_q is introduced to store the current queue node before removing it. This ensures that we can safely restore the node if the block is not freed.

2. Safe Removal of Block References:

   • All elements of the block are removed from the queue (ngx_queue_remove(&iter->chain)) before freeing the block's memory.

3. Restoration of Queue State (if Needed):

   • If the block is not freed (e.g., due to block->used != 0), the queue node is restored using ngx_queue_add.

4. Avoiding Dereference After Free:

   • By fully removing all references to the block before freeing its memory, the patch eliminates the risk of accessing invalid memory.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Start in src/ngx_stream_lua_semaphore.c at ngx_stream_lua_sema_mm_cleanup and inspect how mm->free_queue references semaphore blocks while cleanup removes queue nodes and frees blocks. Verify the proposed queue handling prevents later dereferences of freed memory and preserves queue state for blocks that are not freed.