Skip to content

http2: fix crash on Http2Stream::diagnostic_name() #45123

New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 25, 2022
Merged

http2: fix crash on Http2Stream::diagnostic_name() #45123

merged 1 commit into from
Oct 25, 2022

Conversation

santigimeno
Copy link
Member

It can happen that the Http2Stream::session_ has already been deleted when the Http2Stream destructor is called, causing diagnostic_name() to crash. Observed when running some http2 tests on Windows with the debug logs activated.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 22, 2022 •
edited by santigimeno
Loading

Review requested:

  • @nodejs/http2
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run. labels Oct 22, 2022
RafaelGSS
RafaelGSS reviewed Oct 22, 2022
View reviewed changes
Copy link
Member

@RafaelGSS RafaelGSS left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Wouldn't be better a descriptive log such as "session already destroyed"?

if (sess == nullptr) {
 // new log
} else {
 // actual log
}

not blocking, though

mcollina
mcollina approved these changes Oct 22, 2022
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@santigimeno
http2: fix crash on Http2Stream::diagnostic_name()
d96a37c 
It can happen that the Http2Stream::session_ has already been deleted
when the Http2Stream destructor is called, causing `diagnostic_name()`
to crash. Observed when running some http2 tests on Windows with the
debug logs activated.
@santigimeno santigimeno force-pushed the santi/fix_http2_diag_name_crash branch from 96eae97 to d96a37c Compare October 25, 2022 09:33
@santigimeno
Copy link
Member Author

Wouldn't be better a descriptive log such as "session already destroyed"?

if (sess == nullptr) {
 // new log
} else {
 // actual log
}

not blocking, though

Done!

@santigimeno santigimeno added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 25, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 25, 2022
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/47462/

RafaelGSS
RafaelGSS approved these changes Oct 25, 2022
View reviewed changes
Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

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

LGTM.

@RafaelGSS RafaelGSS added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 25, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 25, 2022
@nodejs-github-bot nodejs-github-bot merged commit 67828d5 into nodejs:main Oct 25, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 67828d5

RafaelGSS pushed a commit that referenced this pull request Nov 1, 2022
@santigimeno @RafaelGSS
http2: fix crash on Http2Stream::diagnostic_name()
2e528fa 
It can happen that the Http2Stream::session_ has already been deleted
when the Http2Stream destructor is called, causing `diagnostic_name()`
to crash. Observed when running some http2 tests on Windows with the
debug logs activated.

PR-URL: #45123
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Nov 1, 2022
Merged
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
@santigimeno @RafaelGSS
http2: fix crash on Http2Stream::diagnostic_name()
1635140 
It can happen that the Http2Stream::session_ has already been deleted
when the Http2Stream destructor is called, causing `diagnostic_name()`
to crash. Observed when running some http2 tests on Windows with the
debug logs activated.

PR-URL: #45123
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@santigimeno @danielleadams
http2: fix crash on Http2Stream::diagnostic_name()
b212b58 
It can happen that the Http2Stream::session_ has already been deleted
when the Http2Stream destructor is called, causing `diagnostic_name()`
to crash. Observed when running some http2 tests on Windows with the
debug logs activated.

PR-URL: #45123
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
@danielleadams danielleadams mentioned this pull request Dec 30, 2022
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@santigimeno @danielleadams
http2: fix crash on Http2Stream::diagnostic_name()
6d78dc5 
It can happen that the Http2Stream::session_ has already been deleted
when the Http2Stream destructor is called, causing `diagnostic_name()`
to crash. Observed when running some http2 tests on Windows with the
debug logs activated.

PR-URL: #45123
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
@santigimeno @danielleadams
http2: fix crash on Http2Stream::diagnostic_name()
a534175 
It can happen that the Http2Stream::session_ has already been deleted
when the Http2Stream destructor is called, causing `diagnostic_name()`
to crash. Observed when running some http2 tests on Windows with the
debug logs activated.

PR-URL: #45123
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

@RafaelGSS RafaelGSS RafaelGSS approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@santigimeno @nodejs-github-bot @mcollina @jasnell @addaleax @cjihrig @RafaelGSS