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 CSNode array overflow issue #724

Closed
wants to merge 1 commit into from
Closed

fix CSNode array overflow issue #724

wants to merge 1 commit into from

Conversation

NKTelnet
Copy link
Contributor

@NKTelnet NKTelnet commented Jun 6, 2019

If a node has not been insert into CSNode array before, when update() is called, we need to call insert() to check whether the CSNode array is full or not.

@ethouris ethouris added Priority: Medium Status: Pending Type: Bug Indicates an unexpected problem or unintended behavior labels Jun 6, 2019
@ethouris
Copy link
Collaborator

ethouris commented Jun 6, 2019

  1. What exactly is the predicted scenario that this function is called without inserting the node into the SndUList first?

  2. Why do you make the first (added) insertion under the critical section, but the second one is outside the critical section?

@NKTelnet
Copy link
Contributor Author

NKTelnet commented Jun 6, 2019

  1. What exactly is the predicted scenario that this function is called without inserting the node into the SndUList first?

there is no SndUList::insert() in whole srtcore code before. Only SndUList::update() will be used.

  1. Why do you make the first (added) insertion under the critical section, but the second one is outside the critical section?

because SndUList::insert() will use the same lock. the lock may be not reentry

@ethouris
Copy link
Collaborator

I saw that inserting is called somewhere in the sending function (in core.cpp). I still don't get what exactly problem this PR solves.

@NKTelnet
Copy link
Contributor Author

I saw that inserting is called somewhere in the sending function (in core.cpp). I still don't get what exactly problem this PR solves.

Could you please show me the code ? I can not find it in master branch:

[root@ srtcore]# grep -r 'insert(' core.cpp
num = m_pSndLossList->insert(losslist_lo, losslist_hi);
num = m_pSndLossList->insert(m_iSndLastAck, losslist_hi);
int num = m_pSndLossList->insert(losslist[i], losslist[i]);
m_pRcvLossList->insert(seqlo, seqhi);
m_FreshLoss.insert(m_FreshLoss.begin() + i + 1, CRcvFreshLoss(next_begin, next_end, m_FreshLoss[i].ttl));
int num = m_pSndLossList->insert(m_iSndLastAck, csn);
int num = m_pSndLossList->insert(m_iSndLastAck, csn);
m_sPollID.insert(eid);
remove.insert(eid);

@ethouris
Copy link
Collaborator

Ok, it was something else. Looxlike "update" is the only function used for it, and "insert" is not in use at all. I must research it myself first.

@NKTelnet
Copy link
Contributor Author

Ok, it was something else. Looxlike "update" is the only function used for it, and "insert" is not in use at all. I must research it myself first.

  1. insert() has not been called.
  2. the update() is wrong no matter whether insert() has been called or not.

@ethouris
Copy link
Collaborator

Ok, now I understand what's going on here.

Your change is perfectly ok, but I'd improve it with a small refactoring:

  1. Change insert into checkEnlarge, remove the mutex lock and a call to insert_. Parameters will be empty in this case.
  2. Simply call it before insert_ in the place where you wanted to call insert and forceUnlock earlier.
  3. After that you can also simplify the structure by using else.

This will at least avoid the unnecessary unlock-lock cycle.

@maxsharabayko maxsharabayko added this to the v.1.3.4 milestone Jul 21, 2019
@maxsharabayko maxsharabayko added the [core] Area: Changes in SRT library core label Aug 12, 2019
@maxsharabayko maxsharabayko modified the milestones: v1.3.4, v1.4.0 Aug 19, 2019
@rndi rndi closed this in #811 Aug 27, 2019
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 Priority: Medium Type: Bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants