Skip to content

Fixed memory leaks with http/3#9342

Merged
bryancall merged 2 commits intoapache:masterfrom
bryancall:h3_memory_leaks2
Feb 7, 2023
Merged

Fixed memory leaks with http/3#9342
bryancall merged 2 commits intoapache:masterfrom
bryancall:h3_memory_leaks2

Conversation

@bryancall
Copy link
Copy Markdown
Contributor

@bryancall bryancall commented Jan 31, 2023

  • Leak with not freeing packets
  • Leak in buffers for QPACK
  • Leak with the http/3 session

These number are with not using the freelist (-f -F):

Before with previous memory leak fixes (#9336):
SUMMARY: AddressSanitizer: 3214562750 byte(s) leaked in 7288765 allocation(s).

After:
SUMMARY: AddressSanitizer: 24670 byte(s) leaked in 429 allocation(s).

@bryancall bryancall added this to the 10.0.0 milestone Jan 31, 2023
@bryancall bryancall requested a review from maskit January 31, 2023 18:10
@bryancall bryancall self-assigned this Jan 31, 2023
Comment thread iocore/net/QUICNet.cc
maskit
maskit previously approved these changes Jan 31, 2023
@bryancall
Copy link
Copy Markdown
Contributor Author

[approve ci]

2 similar comments
@bryancall
Copy link
Copy Markdown
Contributor Author

[approve ci]

@bryancall
Copy link
Copy Markdown
Contributor Author

[approve ci]

@bryancall
Copy link
Copy Markdown
Contributor Author

[approve ci autest]

@maskit
Copy link
Copy Markdown
Member

maskit commented Feb 2, 2023

Doesn't the second commit leak application instance on tests? What was the problem?

@bryancall bryancall requested a review from maskit February 2, 2023 21:46
@bryancall bryancall changed the base branch from 10-Dev to master February 3, 2023 16:57
@bryancall bryancall dismissed maskit’s stale review February 3, 2023 16:57

The base branch was changed.

@maskit
Copy link
Copy Markdown
Member

maskit commented Feb 3, 2023

Seems like changing base branch confuses GitHub. The diff shows code that is already on master.

- Leak with not freeing packets
- Leak in buffers for QPACK
- Leak with the http/3 session
Copy link
Copy Markdown
Member

@maskit maskit left a comment

Choose a reason for hiding this comment

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

Let's leave the ownership issue for now. The change looks fine. Go ahead and merge when autest passes.

@maskit
Copy link
Copy Markdown
Member

maskit commented Feb 7, 2023

[approve ci autest]

@bryancall bryancall merged commit a3aa284 into apache:master Feb 7, 2023
moleksy pushed a commit to moleksy/trafficserver that referenced this pull request Feb 8, 2023
- Leak with not freeing packets
- Leak in buffers for QPACK
- Leak with the http/3 session
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants