Skip to content

Conversation

@scw00
Copy link
Member

@scw00 scw00 commented Aug 10, 2017

This pr try to fix #2323

  • We need to make sure that we remove the correct entry.
  • We need to fix queue if the new node has exclusive flag.

@scw00 scw00 requested review from masaori335 and zwoop August 10, 2017 12:53
@scw00 scw00 self-assigned this Aug 10, 2017
@scw00 scw00 added the HTTP/2 label Aug 10, 2017
@scw00 scw00 added this to the 8.0.0 milestone Aug 10, 2017
@scw00 scw00 changed the title Remove the correct entry from priority queue and insert the new node to the queue Remove the correct entry from priority queue and insert the new node into the queue Aug 10, 2017
parent->children.push(node);
if (!node->queue->empty()) {
parent->queue->push(node->entry);
parent->queued = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be node->queued = true? Because queued flag indicates the node is in the queue or not, if I remember correctly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes ! My mistake.

@scw00
Copy link
Member Author

scw00 commented Aug 11, 2017

Hi @masaori335 , updated please review again!!

Copy link
Contributor

@masaori335 masaori335 left a comment

Choose a reason for hiding this comment

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

👍

@scw00 scw00 modified the milestones: 7.1.1, 8.0.0 Aug 11, 2017
@zwoop
Copy link
Contributor

zwoop commented Aug 11, 2017

Testing this on docs again, will check in the morning if it behaves. Please make sure to add a Project of 7.x for the PRs that are 7.x candidates :-)

@zwoop
Copy link
Contributor

zwoop commented Aug 11, 2017

This looks good now on Docs, I'm gonna merge this and cherry-pick to 7.1.x. Thanks!

@zwoop zwoop merged commit a50aaf1 into apache:master Aug 11, 2017
@zwoop zwoop modified the milestones: 7.1.1, 8.0.0 Aug 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ASAN: heap-use-after-free when enabling H2 priorities

3 participants