Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix memory overflow and capacity regression. #727

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

DrHazemAli
Copy link

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.

Copy link
Author

@DrHazemAli DrHazemAli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed

Copy link
Author

@DrHazemAli DrHazemAli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Followed clang-format rules

@murillo128
Copy link

@mildsunrise could you take a look?

@mildsunrise
Copy link
Contributor

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

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.

3 participants