Conversation
With this commit authorities and additionals in a dns response are parsed.
close2
force-pushed
the
CL/dns-impr
branch
2 times, most recently
from
7ca111a to
b5d9960
Compare
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
WalkthroughThe PR refactors the DNS client API to accept a list of record types instead of a single type, converts Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
lib/net/modules/dns.toit (3)
821-826: Asymmetry between TXT encoding and decoding.
TxtResourceis used for encoding multi-string TXT records (seewrite-resource_at lines 922-929), butdecode-resource_(line 692) createsStringResourcefor decoded TXT records. This asymmetry means:
- Encoding: supports both
TxtResource(multi-string) andStringResource(single string)- Decoding: always produces
StringResource(single string only)If round-trip consistency is needed, consider decoding TXT records as
TxtResourcewhen multiple character-strings are present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/net/modules/dns.toit` around lines 821 - 826, The decode-resource_ path currently converts TXT (RECORD-TXT) records into StringResource even though write-resource_ supports TxtResource (multi-string) and StringResource; update decode-resource_ so that when parsing RECORD-TXT it constructs TxtResource when the rdata contains multiple character-strings (and only use StringResource for single-string cases), ensuring round-trip symmetry between TxtResource, StringResource, write-resource_, and decode-resource_.
271-274: Redundant cache initialization.The
cache_field is initialized twice: once at declaration (line 271) and again in the constructor (line 274). The field initializer::= Mapalready creates the Map instance.♻️ Suggested simplification
abstract class DnsClientBase_ implements DnsClient: cache_ ::= Map // From numeric q-type to (Map name to CacheEntry_). constructor: - cache_ = Map🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/net/modules/dns.toit` around lines 271 - 274, The cache_ field is being double-initialized: it already has a field initializer "cache_ ::= Map" and the constructor also reassigns cache_ = Map; remove the redundant assignment in the constructor (leave the field initializer) or alternatively remove the field initializer and only initialize inside the constructor—pick one approach and keep only a single initialization for cache_ (used to store per q-type Map of name→CacheEntry_) to avoid unnecessary reassignment.
678-692: TXT record decoding only reads the first character-string.The current implementation reads only the first
<length><data>segment of a TXT record. Per RFC 1035, TXT records can contain multiple character-strings. If a TXT record has multiple strings (e.g.,rd-length=12with two 5-byte strings), only the first is decoded and the rest are skipped by theto-skiplogic.This may be intentional for simplicity, but consider whether multi-string TXT records need full support. If so, the loop should consume all character-strings within
rd-length.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/net/modules/dns.toit` around lines 678 - 692, The TXT-record branch (RECORD-TXT) only reads one <character-string> using a single reader.read-byte/read-string and leaves remaining bytes unparsed; change it to loop while rd-length > 0 reading a length octet, validate rd-length >= length+1 each iteration, read that many bytes with reader.read-string and append each string (or collect into a list and join as desired) into the value for the StringResource r-name type ttl flush value, and ensure you decrement/track the consumed bytes so the loop exits when the entire rd-length is consumed; preserve the existing protocol-error_ check for invalid lengths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/net/modules/dns.toit`:
- Around line 922-929: The TxtResource branch writes each TXT string length as a
single byte without validating per-string size, which can overflow since
write-byte accepts only 0-255; update the TxtResource encoding to validate each
string in txt.text (the same way StringResource does) before writing: iterate
txt.text, check s.size <= 255 and raise or return an error if not, then compute
total-len and write each entry using buffer.write-byte s.size and buffer.write
s; ensure the validation occurs prior to buffer-be.write-int16 so total length
isn't computed for invalid entries.
---
Nitpick comments:
In `@lib/net/modules/dns.toit`:
- Around line 821-826: The decode-resource_ path currently converts TXT
(RECORD-TXT) records into StringResource even though write-resource_ supports
TxtResource (multi-string) and StringResource; update decode-resource_ so that
when parsing RECORD-TXT it constructs TxtResource when the rdata contains
multiple character-strings (and only use StringResource for single-string
cases), ensuring round-trip symmetry between TxtResource, StringResource,
write-resource_, and decode-resource_.
- Around line 271-274: The cache_ field is being double-initialized: it already
has a field initializer "cache_ ::= Map" and the constructor also reassigns
cache_ = Map; remove the redundant assignment in the constructor (leave the
field initializer) or alternatively remove the field initializer and only
initialize inside the constructor—pick one approach and keep only a single
initialization for cache_ (used to store per q-type Map of name→CacheEntry_) to
avoid unnecessary reassignment.
- Around line 678-692: The TXT-record branch (RECORD-TXT) only reads one
<character-string> using a single reader.read-byte/read-string and leaves
remaining bytes unparsed; change it to loop while rd-length > 0 reading a length
octet, validate rd-length >= length+1 each iteration, read that many bytes with
reader.read-string and append each string (or collect into a list and join as
desired) into the value for the StringResource r-name type ttl flush value, and
ensure you decrement/track the consumed bytes so the loop exits when the entire
rd-length is consumed; preserve the existing protocol-error_ check for invalid
lengths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bbe3e3bb-2ddd-4ecf-97db-24a8d583c70c
📒 Files selected for processing (7)
lib/net/modules/dns.toitsrc/resources/udp_esp32.ccsrc/resources/udp_win.cctests/dns-tools-test.toittests/dns_cache_retry_test.toittests/dns_mdns_test.toittests/dns_packet_test.toit
💤 Files with no reviewable changes (2)
- src/resources/udp_esp32.cc
- src/resources/udp_win.cc
| if record is TxtResource: | ||
| txt := record as TxtResource | ||
| total-len := 0 | ||
| txt.text.do: | s | total-len += s.size + 1 | ||
| buffer-be.write-int16 total-len | ||
| txt.text.do: | s | | ||
| buffer.write-byte s.size | ||
| buffer.write s |
There was a problem hiding this comment.
Missing length validation for individual TXT strings in TxtResource.
When encoding StringResource TXT records, there's a 255-byte limit check (line 932). However, TxtResource strings (lines 927-929) are written without validating that each individual string doesn't exceed 255 bytes. Since s.size is written as a single byte, strings longer than 255 bytes would cause truncation or incorrect encoding.
🛡️ Proposed fix to add validation
if record is TxtResource:
txt := record as TxtResource
total-len := 0
- txt.text.do: | s | total-len += s.size + 1
+ txt.text.do: | s |
+ if s.size > 255: throw (DnsException "TXT character-string cannot exceed 255 bytes" --name=record.name)
+ total-len += s.size + 1
buffer-be.write-int16 total-len
txt.text.do: | s |
buffer.write-byte s.size
buffer.write s🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/net/modules/dns.toit` around lines 922 - 929, The TxtResource branch
writes each TXT string length as a single byte without validating per-string
size, which can overflow since write-byte accepts only 0-255; update the
TxtResource encoding to validate each string in txt.text (the same way
StringResource does) before writing: iterate txt.text, check s.size <= 255 and
raise or return an error if not, then compute total-len and write each entry
using buffer.write-byte s.size and buffer.write s; ensure the validation occurs
prior to buffer-be.write-int16 so total length isn't computed for invalid
entries.
2 participants
depends on PR #2988
Refactor DnsClient to be an interface. Rename the
get_method and remove the version, which only accepts a single type.