Support EDSN0 packets #13

Open
wants to merge 8 commits into
from

Projects

None yet

2 participants

@kristapsdz
Contributor

This supports EDNS0 with the "owner" draft, which is the only one that appears on my network. There are still issues in that "Unprocessed rr in Additional Section" shows up in debug mode. I'm not sure why.

Kristaps Dzo... and others added some commits Feb 13, 2017
@kristapsdz
Contributor

Sorry, the pledge(2) bits are in here too. I'm new to git.

@haesbaert
Owner
@kristapsdz
Contributor

Done.

@haesbaert
Owner

I can review this today sadly, I'll have a look tomorrow.

But I've added a few ideas.

@haesbaert

Can you squash the two mdnsctl commits into one:
0709f56
and
a596ea7

it's quite simple:

save a backup

git branch bkp

git rebase -i HEAD^^^^^^^^^^
You then change the order of the commits and squash one into the other.
This way you have both commits into one.

mdnsd/packet.c
+ * The T_OPT type is handled differently from other RR types.
+ * See RFC 2671 for details.
+ */
+
@haesbaert
haesbaert Feb 14, 2017 Owner

This reads good to me but I didn't have time to check the RFC yet.

It would be a bit more clean to do something like:

if (rr->rrs.type == T_OPT)
goto switch;

then on the
case T_OPT you do this whole block and return.
This way you can escape the stuff you don't want, and code still looks like a switch

@kristapsdz
kristapsdz Feb 14, 2017 Contributor

Ok, done.

@kristapsdz
kristapsdz Feb 14, 2017 Contributor

Um, except for the rebase thing---I tried and it gave me errors.

mdnsd/packet.c
+ "bytes in RDATA section %zu, "
+ "unknown code %llu",
+ rr_type_name(rr->rrs.type),
+ (long long unsigned)plen, j,
@haesbaert
haesbaert Feb 14, 2017 Owner

why casting plen and code to llu ?
%u and no cast should be enough.

@kristapsdz
kristapsdz Feb 14, 2017 Contributor

Because plen and code are u_int16_t, which should use the % PRIu16, but usually people shit bricks when they see that. Yes, I know that on openbsd, this is %hu or just %u for u_int32_t. But do you really want to discard portability like that? Fixing it later will be a PITA.

@haesbaert
Owner
@haesbaert
Owner
@haesbaert
Owner
@kristapsdz
Contributor
kristapsdz commented Feb 14, 2017 edited

I make it a point to print for the type and not assume (pulling in inttypes.h and using the silly PRIu16 modifiers isn't such a bit deal). Hence the up-cast. IIRC all of the BSDs are ILP32 or I32LP64, but still... I can change to %hu and %u for u_int16_t or u_int32_t with caveat emptor, cast up, or use PRIu16 and PRIu32. (I prefer the latter.) It's up to you!

@haesbaert
Owner
@kristapsdz
Contributor

Done!

+
+ rr->flags = 0;
+ rr->ttl = 0;
+ break;
@haesbaert
haesbaert Feb 15, 2017 edited Owner

This should be a return, if you break you do an extra *len -= rdlen; *pbuf += rdlen; outside the switch on line 1124.

This is new behaviour as your previous diff returned

@kristapsdz
kristapsdz Feb 15, 2017 Contributor

Note I set rdlen to be zero in the heading, then jump to the switch before it's set otherwise. So it should be zero.

@kristapsdz
kristapsdz Feb 15, 2017 Contributor

I just can't figure out where the Unprocessed rr in Additional Section comes in to play after it processes the EDNS0 packets.

@haesbaert
Owner
@kristapsdz
Contributor
kristapsdz commented Feb 15, 2017 edited

Ok, I passed the struct pkt into pkt_parse_rr and can verify that the EDNS0 packets are MDNS_QUERY. Since the pkt_parse_rr is being invoked from the AR RR handler and always inserting into the arlist, should I just not have T_OPT be inserted?

Edit: included in the current version. This obviously gets rid of the warnings; but as it is a query, I'm not sure if that's the correct approach. I just don't know mdns well enough.

@haesbaert
Owner
@haesbaert
Owner
@haesbaert
Owner

I've cherry-picked the mdnsctl bits for now, they're in master now.

@haesbaert haesbaert dismissed their review Feb 18, 2017

dada

@haesbaert
Owner
haesbaert commented Feb 18, 2017 edited

So I've cherry-picked the following:
8100978
b664bbc
db8aab1
4b7f518

I've not merge the commit that caused the leak, neither the fix. I came with something else, please take a look at: 1196417

Can you confirm that now ESDN0 still gets parsed, but you don't get any warning messages ?
If that works for you I'll close this pull request, with I'd like to close v0.7

@haesbaert
Owner

@kristapsdz could you confirm that ESDN0 is still parsed and you don't get the errors now ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment