Skip to content

Use Sendable DNS types.#1269

Draft
jglogan wants to merge 1 commit intoapple:mainfrom
jglogan:dns-records
Draft

Use Sendable DNS types.#1269
jglogan wants to merge 1 commit intoapple:mainfrom
jglogan:dns-records

Conversation

@jglogan
Copy link
Contributor

@jglogan jglogan commented Feb 26, 2026

  • Closes [Request]: Eliminate external DNS library dependencies. #1268.
  • The types we were using weren't very usable with Swift 6 structured concurrency.
  • Implements just the subset of records that we use.
  • Use notImplemented instead of formatError for unknown record types.
  • Use pure actor for LocalhostDNSHandler now that we have sendable types.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Testing

  • Tested locally
  • Added/updated tests
  • Added/updated docs

- Closes apple#1268.
- The types we were using weren't very usable
  with Swift 6 structured concurrency.
- Use notImplemented instead of formatError for
  unknown record types.
- Use pure actor for LocalhostDNSHandler now
  that we have sendable types.

/// The fully-qualified domain name with trailing dot.
public var description: String {
labels.isEmpty ? "." : labels.joined(separator: ".") + "."
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: some lookup paths appear to use plain string (no normalization), will this change cause any issue? For example in AttachmentAllocator;

// store
func allocate(hostname: String).. 

// lookup
func lookup(hostname: String) async throws -> UInt32? {
        hostnames[hostname]
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@manojmahapatra Thanks for looking at this! I converted it to draft.

This one was done by telling Claude to create the types we needed using the bit-fiddling code that ContainerizationNetlink uses, and I pushed it primarily to back the change up and get something started for the issue.

I still need to review the work myself and sanity check it, but I'll bet I would have missed this!

I'll have a look through the relevant RFCs as I review this myself. This isn't an area I'm super expert in so if you've got experience and suggestions I'd welcome them!

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not an expert in this area either, but I reviewed the PR in detail against current behavior. Overall this is moving in the right direction.

I see two gaps worth addressing here:

  • Hostname normalization consistency (e.g. foo vs foo.) across parse/store/lookup paths. (additional focused unit tests will be nice)
  • Compression-pointer safety in DNSName decode to avoid malformed packet loops. Something like this maybe;
// Calculate pointer offset from message start
                let pointerLocation = offset
                let pointer = Int(length & 0x3F) << 8 | Int(buffer[offset + 1])
                let pointerTarget = messageStart + pointer
                guard pointerTarget < pointerLocation else {
                    throw DNSBindError.unmarshalFailure(type: "DNSName", field: "compression pointer not prior")
                }
                offset = pointerTarget

@jglogan jglogan marked this pull request as draft March 4, 2026 03:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Request]: Eliminate external DNS library dependencies.

2 participants