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

Potential CSndUList array overflow #811

Merged
merged 2 commits into from
Aug 27, 2019

Conversation

maxsharabayko
Copy link
Collaborator

@maxsharabayko maxsharabayko commented Aug 13, 2019

This is a rework of #724 (closes #724).
The default size of CSndUList was 4096 elements. The list is a heap of SRT sockets, that are to be processed in the sender's thread.
It is very unlikely to have mare than 4096 SRT connections, that is why checking if there is a place to insert a new socket is not required most of the time. However, if there is no place, then there will be an overflow and out-of-border operations.
This PR fixes this.

  • CSndUList::update(...) increases list size if required.
  • Reduced default size of the CSndUList from 4096 to 512.
  • CSndUList::remove_(...) now uses std::swap
  • CSndUList::insert_norealloc_(...) now uses std::swap

@maxsharabayko maxsharabayko added the [core] Area: Changes in SRT library core label Aug 13, 2019
@maxsharabayko maxsharabayko added this to the v1.3.4 milestone Aug 13, 2019
}
catch (...)
{
return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's not the best approach to handle the memory allocation problems. You simply do nothing and behave as if allocation succeeded. If this is a proper and predicted memory allocation error handling, please at least leave a comment that describes how it is handled. Trouble is, things like "double the size" for allocation - unlike usual critical memory resource allocation - should be predicted for temporary resource problems. For example, a socket that fails to be added to this list should be somehow postponed or even closed, if there's no other choice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

CSndBuffer throws CUDTException(MJ_SYSTEMRES, MN_MEMORY, 0). Probably the same has to be done here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, I thought it can't be done, but indeed - the only functions from public API of this class is remove and update, and update is the only call that can cause this problem is from CUDT::sendfile and CUDT::sendmsg2.

Maybe it would be even better idea to move the insert function to a private section? If not even remove... When I did so for testing, the code still compiles.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

insert is private now. Renamed to insert_ and insert_norealloc_ (according with the naming of the class) and removed the mutex lock.

srtcore/udt.h Outdated Show resolved Hide resolved
@maxsharabayko maxsharabayko force-pushed the hotfix/csnode-overflow branch 2 times, most recently from 0906051 to b3e24fd Compare August 13, 2019 16:37

try
{
temp = new CSNode * [m_iArrayLength * 2];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe remove spaces around '*'? The syntax is confusing enough, spaces make it look even more like multiplication. This is, obviously, an opinion, feel free to ignore if your opinion differs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

A kinda standard in C++ is that there is no space between type name and * for pointer types (or even more precisely, to glue the asterisk rather to preceding type name than to a following symbol name - which isn't exactly the case here though). Spaces around * as a multiplication operator is another matter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Applied CLang format from #772 to this line of code.
Changed
temp = new CSNode * [m_iArrayLength * 2];
to
temp = new CSNode *[2 * m_iArrayLength];

srtcore/queue.cpp Show resolved Hide resolved
@maxsharabayko maxsharabayko modified the milestones: v1.3.4, v1.4.0 Aug 19, 2019
@ethouris ethouris added Status: Review Needed Type: Bug Indicates an unexpected problem or unintended behavior labels Aug 22, 2019
@rndi rndi merged commit c0630e8 into Haivision:master Aug 27, 2019
@FedericoCeratto
Copy link
Contributor

FedericoCeratto commented Sep 1, 2019

This vulnerability is tracked in CVE-2019-15784
@ethouris: Do you plan to make a new release soon? Thanks

@maxsharabayko maxsharabayko deleted the hotfix/csnode-overflow branch March 9, 2021 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[core] Area: Changes in SRT library core Type: Bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants