Proposal: Decorate "overflow unpacking" errors with where in the unpacking the error occurred.

[EricIO]

I'm quite willing to do this work myself but wanted to first check if it is something that might get accepted.

We're seeing overflow unpacking uint16 errors in our system and are having trouble reproducing it locally / getting a packet capture to see what is going on. One thing that might help us pinpoint what is going on would be some better logging.

The proposal would be to decorate errors from the unpackUint* and related family of functions with where the error occurred. For example:

	hdr.Rrtype, off, err = unpackUint16(msg, off)
	if err != nil {
		return hdr, len(msg), msg, err
	}

to

	hdr.Rrtype, off, err = unpackUint16(msg, off)
	if err != nil {
		return hdr, len(msg), msg, fmt.Errorf("header.Rrtype: %w", err)
	}

[bkr-JNPR]

@EricIO , I am also seeing the same overflow issue in this package, issue happens rarely, dns packet looks fine.
not able to find out which DNS contents getting corrupted or some other reason.

please let me know if u already know more information .

[miekg]

That looks useful. Needs maybe some kind of naming convention to tell where the error originated and some tweaks to make this work for all the autogenerated code. [ Quoting ***@***.***> in "[miekg/dns] Proposal: Decorate "ove..." ]

I'm quite willing to do this work myself but wanted to first check if it is something that might get accepted first. We're seeing overflow unpacking uint16 errors in our system and are having trouble reproducing it locally / getting a packet capture to see what is going on. One thing that might help us pinpoint what is going on would be some better logging. The proposal would be to decorate errors from the unpackUint* and related family of functions with where the error occurred. For example: hdr.Rrtype, off, err = unpackUint16(msg, off) if err != nil { return hdr, len(msg), msg, err } to hdr.Rrtype, off, err = unpackUint16(msg, off) if err != nil { return hdr, len(msg), msg, fmt.Errorf("header.Rrtype: %w", err) } — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you are subscribed to this thread.*Message ID: ***@***.***>

/Miek

…

-- Miek Gieben

[EricIO]

Great. I'll try and have a draft PR up by the end of the weekend.

[bkr-JNPR]

@miekg , as @EricIO mentioned, it is hard to recreate the issue, It seems this happens during parsing of DNS response records , I see packet capture during this overflow error, Wireshark says DNS packets are completely fine and contain up to 15 resource records in DNS response packet .
When wireshark says a packet is proper, how does the DNS parser say overflow? This is the confusion .

[miekg]

yes, this may turn up a bug or two in code, or not

[bkr-JNPR]

@EricIO /@miekg,
any update on the patch for this issue ?

[EricIO]

@bkr-JNPR Had some stuff come up so haven't had time to work on finishing it. Hopefully will soon.

[bkr-JNPR]

@EricIO ,
Thanks. I hope you will make the change as soon as possible, don't want to duplicate the work .

[EricIO]

I'm currently working on the generated code to get that in a nice format. Will be on vacation a couple days so no progress will be made until I get back.