Skip to content

Fix WKS record protocol field encoding.#220

Open
donapieppo wants to merge 1 commit into
alexdalitz:masterfrom
donapieppo:master
Open

Fix WKS record protocol field encoding.#220
donapieppo wants to merge 1 commit into
alexdalitz:masterfrom
donapieppo:master

Conversation

@donapieppo

Copy link
Copy Markdown

Fix WKS record protocol field encoding and decoding size according to RFC 1035 Section 3.4.2 where PROTOCOL is an 8 bit.
I spotted the errror using Dnsruby::ZoneTransfer where i got the error

Dnsruby::MessageDecoder#assert_buffer_position_valid': requested position of 1871 must be     
  between 0 and buffer size (1870). (Dnsruby::DecodeError) 

The patch just change the pack format from "n" to "C" in lib/dnsruby/resource/IN.rb

… RFC 1035 Section 3.4.2 where PROTOCOL is an 8 bit
@keithrbennett

keithrbennett commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Hi, I have no knowledge of this particular domain, but asked Claude AI about it, and had its response reviewed successfully by Qwen AI. TL/DR is that the fix is correct, and in addition this would be a good opportunity to add unit tests for the feature. Suggested test code is also included in the response:

https://claude.ai/share/e0d07f8a-f26b-4548-a0db-71b12a7ee817

(EDIT: The source code was not included in the text referred to by the link; I have pasted it in the comment following this one.)

Qwen review at: https://chat.qwen.ai/s/58977257-7fe0-4b11-8307-c4d0196c2a69?fev=0.2.64

@alexdalitz I realize some people would prefer not to receive comments like these where I sanity checked the information but did not deeply verify it. If you would prefer me not post similar posts in the future, just say so and I will refrain.

@keithrbennett

Copy link
Copy Markdown
Contributor

Sorry, I could not get a link to the conversation that included the test code. Here it is, with explanation following:

# test/tc_wks.rb
require_relative 'spec_helper'

class WKSTest < Minitest::Test
  # Protocol 6 = TCP, per IANA; fits in 8 bits (0-255).
  # Bitmap marks ports 80 (HTTP) and 443 (HTTPS) as available.
  ADDR     = "192.0.2.1"
  PROTOCOL = 6
  BITMAP   = "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02".b

  def wks
    Dnsruby::RR::IN::WKS.new(
      Dnsruby::IPv4.new(ADDR),
      PROTOCOL,
      BITMAP
    )
  end

  def encode(rr)
    msg = Dnsruby::MessageEncoder.new
    rr.encode_rdata(msg)
    msg.to_s
  end

  def roundtrip(rr)
    encoded = encode(rr)
    # The encoded wire format for WKS is: 4 bytes address + 1 byte protocol + bitmap.
    # With the old "n" (2-byte) format this was 6 + bitmap; now it must be 5 + bitmap.
    assert_equal 4 + 1 + BITMAP.bytesize, encoded.bytesize,
      "wire format must be 4-byte address + 1-byte protocol + bitmap"

    decoder = Dnsruby::MessageDecoder.new(encoded)
    Dnsruby::RR::IN::WKS.decode_rdata(decoder)
  end

  def test_roundtrip_preserves_protocol
    decoded = roundtrip(wks)
    assert_equal PROTOCOL, decoded.protocol
  end

  def test_roundtrip_preserves_address
    decoded = roundtrip(wks)
    assert_equal ADDR, decoded.address.to_s
  end

  def test_roundtrip_preserves_bitmap
    decoded = roundtrip(wks)
    assert_equal BITMAP, decoded.bitmap
  end

  def test_protocol_field_is_one_byte
    # Regression: the old "n" pack format wrote 2 bytes for protocol,
    # causing MessageDecoder to overshoot the buffer by 1 byte (Dnsruby::DecodeError).
    encoded = encode(wks)
    # Byte 4 (0-indexed) must be the protocol value itself, not the high byte of a 16-bit field.
    assert_equal PROTOCOL, encoded.bytes[4]
  end

  def test_max_protocol_value
    rr = Dnsruby::RR::IN::WKS.new(Dnsruby::IPv4.new(ADDR), 255, BITMAP)
    decoded = roundtrip(rr)
    assert_equal 255, decoded.protocol
  end
end

A few notes on the choices:

test_protocol_field_is_one_byte is the regression test that would have caught the original bug — it directly asserts that byte index 4 of the wire encoding carries the protocol value, which fails with the old "n" format (which would write 0x00 there and the protocol value at byte 5).

test_roundtrip_* are the correctness tests: encode then decode and verify all three fields survive intact. Splitting them into three assertions rather than one compound one gives clearer failure messages.

test_max_protocol_value exercises the boundary (255) to confirm the 8-bit field handles the full valid range.

The BITMAP constant is intentionally non-trivial (marks real ports) to ensure bitmap content also survives the round-trip without corruption from an off-by-one in buffer consumption.

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.

2 participants