Fix memory overflow and capacity regression.

[DrHazemAli]

Fix Memory Overflow and Capacity Regression in srtp_stream_list_insert

This pull request addresses a critical issue in the srtp_stream_list_insert function where potential integer overflow and capacity regression could occur during memory allocation.

Problem

The original code attempted to validate the multiplication of sizeof(list_entry) and new_capacity to check for overflow. However, this approach was flawed because the multiplication itself could overflow before the comparison, leading to undefined behavior.

Solution

The fix ensures:

1. Prevention of Integer Overflow:
   • Validates new_capacity against SIZE_MAX / sizeof(list_entry) before performing the multiplication.
2. Prevention of Capacity Regression:
   • Adds a check to ensure new_capacity does not regress below the current capacity, safeguarding against unintentional regressions.

Updated Code

The following changes were made in the srtp_stream_list_insert function:

size_t new_capacity = list->capacity * 2;

// Check for capacity overflow.
// Fix: Ensure new_capacity does not regress below current capacity
// and prevent integer overflow during memory allocation.
// The multiplication (sizeof(list_entry) * new_capacity) is validated
// by checking that new_capacity is less than SIZE_MAX / sizeof(list_entry).
if (new_capacity < list->capacity || 
    new_capacity > SIZE_MAX / sizeof(list_entry)) {
    return srtp_err_status_alloc_fail;
}

Impact

1. Improved Memory Safety:
   • Eliminates the risk of undefined behavior caused by integer overflow during memory allocation.
2. Enhanced Robustness:
   • Ensures that new_capacity always increases or remains valid, preventing capacity regression.

Testing

• Verified the fix on multiple scenarios to ensure correctness and performance.
• Passed all existing tests for srtp_stream_list_insert without any regressions.

Additional Notes

• The fix adheres to standard C programming best practices for safe memory management.
• This change does not introduce new dependencies or significant overhead.

[DrHazemAli]

Reviewed

[DrHazemAli]

Followed clang-format rules

[murillo128]

@mildsunrise could you take a look?

[mildsunrise]

thanks for the ping! looks okay to me. we're not affected though, right?

[pabuhler]

@DrHazemAli I am not sure why the format still fails :( but changing to :

@@ -4899,7 +4899,8 @@ srtp_err_status_t srtp_stream_list_insert(srtp_stream_list_t list,
         size_t new_capacity = list->capacity * 2;
 
         // check for capacity overflow.
-        if (new_capacity < list->capacity || new_capacity > SIZE_MAX / sizeof(list_entry)) {
+        if (new_capacity < list->capacity ||
+            new_capacity > SIZE_MAX / sizeof(list_entry)) {
             return srtp_err_status_alloc_fail;
         }

did fix it for me locally. Could you please update it one more time, thanks!

[DrHazemAli]

Updated

[DrHazemAli]

clang-formatted.

[DrHazemAli]

@pabuhler It turned out that the issue is related to c-lang code formatting, can you give it a try?

[pabuhler]

@DrHazemAli, not sure what happened but I reverted your last commit and fixed the formatting. If you are Ok with this then we can merge it.
Maybe squashing to remove some of this history ?

[DrHazemAli]

Looks okay!
Reviewed ✅

[DrHazemAli]

@pabuhler I looked to your commit it looks good!
Let's merge it.