Skip to content

net/mbedtls_net_connect: Preventing double close problem#9711

Merged
davidhorstmann-arm merged 2 commits intoMbed-TLS:developmentfrom
ThePassionate:mbedtls-1021
Oct 22, 2024
Merged

net/mbedtls_net_connect: Preventing double close problem#9711
davidhorstmann-arm merged 2 commits intoMbed-TLS:developmentfrom
ThePassionate:mbedtls-1021

Conversation

@ThePassionate
Copy link
Copy Markdown

@ThePassionate ThePassionate commented Oct 21, 2024

In the test examples and real usage scenarios, 'mbedtls_net_free' is called after 'mbedtls_net_connect' fails, which will cause the problem of double close the same fd. It is possible to close this closed fd which has been applied by other link.

Description

hi all. We found that in the scenario of sudden network disconnection, ‘mbedtls_net_connect’ will close fd after the connection fails, and then call ‘mbedtls_net_free’ to close it again. There is a risk of double close here. At first, we suspected that it was used incorrectly, but after referring to the implementation and case in the programs directory, we did not find that it was a usage error. Therefore, we suggest to mark fd as unavailable after closing it to prevent fd from being used by other programs during the interval between 'mbedtls_net_connect' and 'mbedtls_net_free'.

PR checklist

Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line.
If the provided content is part of the present PR remove the # symbol.

Resolves #9710

Notes for the submitter

Please refer to the contributing guidelines, especially the
checklist for PR contributors.

Help make review efficient:

  • Multiple simple commits
    • please structure your PR into a series of small commits, each of which does one thing
  • Avoid force-push
    • please do not force-push to update your PR - just add new commit(s)
  • See our Guidelines for Contributors for more details about the review process.

Copy link
Copy Markdown
Contributor

@davidhorstmann-arm davidhorstmann-arm left a comment

Choose a reason for hiding this comment

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

Change looks good to me, thanks!

Would you be able to add a ChangeLog entry and raise backport PRs against the mbedtls-3.6 and mbedtls-2.28 branches?

For ChangeLog guidance, see the ChangeLog readme. This would count in the Bugfix category.

@davidhorstmann-arm davidhorstmann-arm added needs-review Every commit must be reviewed by at least two team members, needs-backports Backports are missing or are pending review and approval. component-platform Portability layer and build scripts needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review size-xs Estimated task size: extra small (a few hours at most) labels Oct 21, 2024
@ThePassionate ThePassionate force-pushed the mbedtls-1021 branch 2 times, most recently from e549a49 to eb77422 Compare October 22, 2024 02:15
@ThePassionate ThePassionate reopened this Oct 22, 2024
@ThePassionate
Copy link
Copy Markdown
Author

Change looks good to me, thanks!

Would you be able to add a ChangeLog entry and raise backport PRs against the mbedtls-3.6 and mbedtls-2.28 branches?

For ChangeLog guidance, see the ChangeLog readme. This would count in the Bugfix category.

Thanks for your reply! Changelog has been added, and backport PRs has been submitted with #9714 and #9715. Thanks again.

In the test examples and real usage scenarios, 'mbedtls_net_free' is called after 'mbedtls_net_connect' fails, which will cause the problem of double close the same fd. It is possible to close this closed fd which has been applied by other link.

Signed-off-by: makejian <makejian@xiaomi.com>
@gowthamsk-arm gowthamsk-arm removed the needs-backports Backports are missing or are pending review and approval. label Oct 22, 2024
Copy link
Copy Markdown
Contributor

@davidhorstmann-arm davidhorstmann-arm left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@gowthamsk-arm gowthamsk-arm self-requested a review October 22, 2024 11:08
@gowthamsk-arm gowthamsk-arm removed the needs-reviewer This PR needs someone to pick it up for review label Oct 22, 2024
gowthamsk-arm
gowthamsk-arm previously approved these changes Oct 22, 2024
Copy link
Copy Markdown
Contributor

@gowthamsk-arm gowthamsk-arm left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Contributor

@davidhorstmann-arm davidhorstmann-arm left a comment

Choose a reason for hiding this comment

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

The CI is failing on the other PRs as the ChangeLog needs a newline at the end. Otherwise looks good to me.

Signed-off-by: David Horstmann <david.horstmann@arm.com>
Copy link
Copy Markdown
Contributor

@davidhorstmann-arm davidhorstmann-arm left a comment

Choose a reason for hiding this comment

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

LGTM, approving even though this technically includes my own change as the change is a line-ending only.

@ThePassionate
Copy link
Copy Markdown
Author

The CI is failing on the other PRs as the ChangeLog needs a newline at the end. Otherwise looks good to me.

Thanks, i will fix that in backport PRs.

@davidhorstmann-arm davidhorstmann-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Oct 22, 2024
@davidhorstmann-arm davidhorstmann-arm added this pull request to the merge queue Oct 22, 2024
Merged via the queue into Mbed-TLS:development with commit f0abbb4 Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Design and code approved - may be waiting for CI or backports component-platform Portability layer and build scripts size-xs Estimated task size: extra small (a few hours at most)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mbedtls_net_connect connect failed

3 participants