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

Ftp::Gateway may segfault in level-3 double-complete debugs() #1923

Conversation

eduard-bagdasaryan
Copy link
Contributor

@eduard-bagdasaryan eduard-bagdasaryan commented Oct 28, 2024

Ftp::Gateway::completeForwarding() must check data.conn pointer before
dereferencing it. Long-term, we should improve Comm::ConnectionPointer
printing to safely report Connection::id (where available). This minimal
fix just mimics existing Ftp::Relay::abortOnData() solution.

Ftp::Gateway::completeForwarding() must check that both Control and Data
connections are still available before dereferencing them.
@rousskov rousskov changed the title Ftp::Gateway segfaults during the double-complete check Ftp::Gateway may segfault in level-3 double-complete debugs() Oct 28, 2024
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

Thank you for fixing this bug! I have adjusted PR title and description to highlight that this bug is specific to level-3 debugging, may not be triggered in every double-complete case, and that the ugly solution mimics existing Ftp::Relay::abortOnData() code. Please adjust further as needed.

Long-term, we should do better, but this minimal fix is enough for now.

@rousskov rousskov added M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels S-could-use-an-approval An approval may speed this PR merger (but is not required) labels Oct 28, 2024
@yadij yadij removed the S-could-use-an-approval An approval may speed this PR merger (but is not required) label Oct 28, 2024
@rousskov rousskov added the S-could-use-an-approval An approval may speed this PR merger (but is not required) label Oct 28, 2024
@rousskov
Copy link
Contributor

@yadij, S-could-use-an-approval label remains useful until "PR approval" check becomes green. In this particular case, after your approval (thank you), that label will remain useful for about 44 hours (unless Francesco approves before the fast track objection timer expires). I have restored that label.

@kinkie kinkie removed the S-could-use-an-approval An approval may speed this PR merger (but is not required) label Oct 29, 2024
squid-anubis pushed a commit that referenced this pull request Oct 29, 2024
Ftp::Gateway::completeForwarding() must check data.conn pointer before
dereferencing it. Long-term, we should improve Comm::ConnectionPointer
printing to safely report Connection::id (where available). This minimal
fix just mimics existing Ftp::Relay::abortOnData() solution.
@squid-anubis squid-anubis added the M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Oct 29, 2024
@squid-anubis squid-anubis added M-merged https://github.com/measurement-factory/anubis#pull-request-labels and removed M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels labels Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
M-merged https://github.com/measurement-factory/anubis#pull-request-labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants