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

netdb:Add macro to control DNS using TCP transmission #15069

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yjq91115
Copy link

@yjq91115 yjq91115 commented Dec 6, 2024

Summary

When dns uses tcp for transmission, it is easy to exceed RECV_BUFFER_SIZE, causing resolution failure. Therefore a macro was added to limit whether this feature is enabled or not.

Impact

Update this section, where applicable, on how change affects users,
build process, hardware, documentation, security, compatibility, etc.

Testing

Update this section with details on how did you verify the change,
what Host was used for build (OS, CPU, compiler, ..), what Target was
used for verification (arch, board:config, ..), etc. Providing build
and runtime logs from before and after change is highly appreciated.

@github-actions github-actions bot added Area: OS Components OS Components issues Size: S The size of the change in this PR is small labels Dec 6, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 6, 2024

[Experimental Bot, please feedback here]

Does this PR meet the NuttX Requirements?

No. The provided information is insufficient to determine if the PR meets the NuttX requirements. While the summary explains the what and why, it lacks the how. The impact and testing sections are essentially placeholders.

Specifically lacking:

  • Summary: Missing a detailed explanation of how the macro works, how it's configured, and its default state. Also missing related issue references.
  • Impact: All impact areas need to be addressed with "YES" or "NO" and a description if "YES." The current placeholder text needs to be replaced with specific details. Crucially, the impact on the user needs to be clarified. Will they need to explicitly enable this TCP DNS feature? How?
  • Testing: Completely missing. Needs details about the build host (OS, CPU, compiler version), the target platform (architecture, board, configuration), and, most importantly, the actual test logs from before and after the change. Simply stating that it "works as intended" is insufficient. Show the failure before the change and the success after. Demonstrate that the macro effectively controls the behavior.

To make this PR acceptable, the author needs to:

  1. Expand the Summary: Explain the mechanism of the new macro. How is it defined? How is it used to enable/disable the TCP DNS feature? What's the default setting? Link any relevant NuttX issues.

  2. Complete the Impact Assessment: Replace the placeholder text with concrete answers (YES/NO) and descriptions for all impact categories. Pay particular attention to the user impact: how will they interact with this new macro?

  3. Provide Thorough Testing Information: Specify the build host and target details. Include actual log output demonstrating the issue before the change, and the resolved issue after the change. Show how the macro controls the behavior. Ideally, include tests that demonstrate both the enabled and disabled states of the feature.

@xiaoxiang781216
Copy link
Contributor

please remove Gerrit Change-ID's before submitting upstream.

@yjq91115
Copy link
Author

yjq91115 commented Dec 6, 2024

please remove Gerrit Change-ID's before submitting upstream.

Done.

@@ -237,6 +237,11 @@ config NETDB_DNSSERVER_IPv4ADDR
Default DNS server IPv4 address in host byte order. Default value
10.0.0.1. This may be changed via dns_add_nameserver().

config NETDB_DNS_STREAM
Copy link
Contributor

Choose a reason for hiding this comment

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

the original code already support both TCP/UDP, why do you make this patch?

Copy link
Author

Choose a reason for hiding this comment

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

When DNS resolution returns the truncate response, this function will establish TCP connection and return the complete response. However, the complete response can easily exceed the RECV_BUFFER_SIZE, causing the resolution to fail, so I wrote a simple solution to solve this problem. There are two better solutions. One is to recycle the buffer and finally splice the buffer. The second is to use molloc to create a length that matches the response. I would like to ask if there is a better solution. @yamt
ping failed
ping failed.log

Copy link
Contributor

Choose a reason for hiding this comment

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

the simplest solution, if it's acceptable for your configuration, would be to bump your CONFIG_NETDB_DNSCLIENT_MAXRESPONSE.

i agree it makes sense to try to use malloc when it doesn't fit RECV_BUFFER_SIZE.
it shouldn't be too difficult to make our client decide after receiving the record size (the first two bytes of the response) with tcp.

i'm not sure how difficult buffer recycling is.

depending on your configurations and environment, you might also want to implement EDNS buffer size.
some servers "tweak" their responses to fit the specified size.

Copy link
Author

Choose a reason for hiding this comment

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

thank you for your suggestion!

config NETDB_DNS_STREAM
bool "DNS supports TCP transmission"
depends on LIBC_NETDB
default n
Copy link
Contributor

Choose a reason for hiding this comment

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

Please include a --help-- to explain exactly what is this feature, when it should be used, etc

Copy link
Author

Choose a reason for hiding this comment

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

Done

@yjq91115 yjq91115 force-pushed the dnsstream branch 2 times, most recently from 0e53657 to 165528e Compare December 9, 2024 11:40
Copy link
Contributor

@yamt yamt left a comment

Choose a reason for hiding this comment

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

When dns uses tcp for transmission, it is easy to exceed RECV_BUFFER_SIZE, causing resolution failure. Therefore a macro was added to limit whether this feature is enabled or not.

well, tcp is used only when the response is truncated in the first place.
thus, disabling tcp does not solve resolution failure problems you are seeing at all.
am i missing something?

@yjq91115
Copy link
Author

When dns uses tcp for transmission, it is easy to exceed RECV_BUFFER_SIZE, causing resolution failure. Therefore a macro was added to limit whether this feature is enabled or not.

well, tcp is used only when the response is truncated in the first place. thus, disabling tcp does not solve resolution failure problems you are seeing at all. am i missing something?

Actually, when tcp is turned off, the truncate reply can be resolve normally.
tcp closed.log
tcp closed

@yamt
Copy link
Contributor

yamt commented Dec 10, 2024

When dns uses tcp for transmission, it is easy to exceed RECV_BUFFER_SIZE, causing resolution failure. Therefore a macro was added to limit whether this feature is enabled or not.

well, tcp is used only when the response is truncated in the first place. thus, disabling tcp does not solve resolution failure problems you are seeing at all. am i missing something?

Actually, when tcp is turned off, the truncate reply can be resolve normally. tcp closed.log tcp closed

ok.
if you are lucky, the truncated response might happen to contain enough info.
i consider it a bug to rely on that though.
cf. https://www.rfc-editor.org/rfc/rfc2181#section-9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: OS Components OS Components issues Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants