Packet Api Rewrite

So, here it is #8 (closed) and more in the wiki page. It's a lot of code, but feel free to ask anything.

Here's the general overview of the state of things:

• Packet API
  • Streamlined, compression reworked, checks in one place, ...
• Query processing
  • Basically everything from the old name-server.c was remade so it stacks
  1. name-server.h and the ns_proc_* API, which is an API for a state machine that just accepts wireformat and does all the conversions between parsed packets and backwards.
  2. ns_proc_query.h is the implementation of query processing and implements the ns_proc_* API
  • It serves as a generic query -> response machine using solvers.
  • It also covers transaction security and stuff, so the solvers don't need to worry about this.
  3. internet.h, chaos.h, axfr.h, ixfr.h, update.h, notify.h
  • Solver implementations for various queries
    • IN answers / DNSSEC proofs are now RFC compliant in several cases
    • Transfers now also show message and byte counts
  • Since all the bitpushing is done by the upper layers, they just do answer solving
• UDP & TCP handlers
  • Simplified, using memory context for query processing
  • TCP threads are now coherent, no through-the-pipe "fair" queueing
• XFR handler is simplified as well (all threads coherent)
• Basic unit tests for packet, query processing and stuff
• I don't know what else, just ask.

Pitfalls

• UPDATE forwarding is disabled until #189 (closed) is finished, as to not create more duplicated code and this is already complex enough.

Review

0.0% src/knot/ 0.0% src/knot/conf/ 0.0% src/knot/stat/ 0.0% src/libknot/dnssec/ 0.0% tests-extra/data/ 0.0% tests-extra/tests/basic/nsec/ 0.0% tests-extra/tests/basic/nsec3/ 0.0% tests-extra/tests/security/protos/ 0.1% src/ 0.4% src/utils/nsupdate/ 0.7% src/common/ 1.2% src/utils/dig/ 1.4% src/utils/common/ 1.5% src/libknot/ 1.8% src/knot/ctl/ 2.3% src/libknot/zone/ 3.1% src/libknot/updates/ 3.5% tests/ 4.2% src/libknot/util/ 15.5% src/knot/server/ 25.4% src/libknot/packet/ 37.8% src/libknot/nameserver/ (with the exception of nsec3_proofs)

jvcelak: libknot/packet/pkt.h -> libknot/packet/packet.h . But fine... mvavrusa: nope jvcelak:

jvcelak: pkt.h: almost all packet flags start with KNOT_PF_, why does KNOT_PACKET_DUPL_NO_MERGE not? mvavrusa: resolved in 48cecedb

jvcelak: knot_pkt_new - I'm thinking about a separate function for allocation. If you incidentally supply non-initialized variable as the wire parameter, you may get into trouble. Having a separate function for allocation would be safer. mvavrusa: I don't see how that helps, if you supply non-initialized variables as the wire you're screwed anyway? jvcelak: I meant that with the current approach you expect initialization. If there is a separate function for allocation, the purpose will be clear from the function's name. But not a big deal.

jvcelak: knot_pkt_reset() you can use offsetof instead of (size_t)&((knot_pkt_t*)NULL)->rr_info); jvcelak: fixed

jvcelak: knot_pkt_type() : bool is_query = (knot_wire_get_qr(pkt->wire) == 0); reverse logic? if QR is set, the returned value will be > 0 jvcelak: invalid, sorry I thought the interpretation of QR is reverse

jvcelak: knot_pkt_opt_set() - I would rather see a separate function for each option. But fine. mvavrusa: I'd like to leave it to #190 (closed) , more work is needed on OPT... jvcelak: ok

jvcelak: knot_pkt_put_question: small copy&paste size_t question_len = 2 * sizeof(uint16_t) + qname_len; mvavrusa: I don't see it, but I've changed it a bit in 056c64c7 jvcelak: resolved

jvcelak: knot_pkt_question_size() includes a size of the header ... it's not really a question size jvcelak: the same with knot_pkt_parse_question() - it also parses the header. mvavrusa: knot_pkt_question_size() is reworked, knot_pkt_parse_question() has updated doc to reflect that it checks sanity of the header as well a5738f67 (I don't think it makes sense to separate question and header parsing, as it's a glorified size check) jvcelak: agreed

jvcelak: knot_pkt_put_opt() should probably check a current section, should not it? mvavrusa: I'd like to leave it to #190 (closed) , more work is needed on OPT... but I put a check there anyway 17cba1c8 jvcelak:

jvcelak: int knot_pkt_put(knot_pkt_t *pkt, uint16_t compress, const knot_rrset_t *rr, uint32_t flags); Parameter flags has type uint32_t, but is assigned into uint16_t variable in knot_rrinfo_t jvcelak: fixed

jvcelak: in knot_pkt_put, in the check for a duplicated RR set: pkt_contains(pkt, rr, KNOT_RRSET_COMPARE_PTR) will evaluate to true in case of error. The input parameters are not checked, so it can easily happen. (In addition, the assignments to rr_info and rr can me moved after the duplicated RR set check.) jvcelak: fixed

jvcelak: you do not use get in getters, rename knot_pkt_get_last -> knot_pkt_last_rrset for consistency? jvcelak: if the function is legacy, can we add deprecated attribute? mvavrusa: function is deprecated, removed

jvcelak: generally, some public knot_pkt_* API functions use asserts for parameter checking jvcelak: fixed

jvcelak: There are also some inconsistencies in types naming: knot_packet_type_t -> knot_pkt_type_t, knot_section_t -> knot_pkt_section_t ? mvavrusa: I like knot_pkt_type_t, but knot_pkt_section_t is too long mvavrusa: knot_packet_type_t renamed and moved, knot_section_t left in consts unchanged in 48cecedb @jvcelak ? jvcelak: perfect

jvcelak: knot_packet_type_t: there is no constant for value 0, I guess it should be. And the usage of flag-like values is quite strange. mvavrusa: KNOT_QUERY_INVALID is the new 0, they are flag-like because response is always odd, query is even (KNOT_RESPONSE is 1) see 48cecedb jvcelak:

jvcelak: knot_rrset_header_to_wire, if (compr && compr->rrinfo->compress_ptr[0] > 0) -> if (compr && compr->rrinfo->compress_ptr[0] != COMPR_HINT_NONE) ? mvavrusa: same thing, depends on whether you view compress_ptr as packet offset or a "hint" (special number < HEADER_SIZE), I'll leave it up to your consideration - jvcelak: ok, let's keep it this way.

jvcelak: instead of adding new fields to be zeroed in knot_rrset_new, I would rather use calloc and set only the fields, which has to be set. IMHO it is more error proof. mvavrusa: I'll leave this for rrset/node refactoring

jvcelak: knot_pkt_tsig_set sets a new key, but does not remove a reservation for the old one. mvavrusa: solved in 056c64

jvcelak: I'm looking at rrl_slip_roll, we probably discussed it some time ago but I do not recall what was the result. Function knot_random_uint16_t is quite slow. Is the rrl_slip_roll called for each positive response when RRL is enabled? Won't it create unwanted performance drop for non-slipped replies? mvavrusa: it is slower, but so is whole RRL processing code for each packet, I don't think it matters . jvcelak: lawyered

jvcelak: memset(&tcp.query_ctx, 0, sizeof(tcp.query_ctx)); in tcp_master() is no-op, as the tcp structure is zeroed a few lines before jvcelak:

jvcelak: axfr_process_item has confusing and too generic name, what about axfr_process_node? mvavrusa: fine, fine 9884e06e jvcelak:

jvcelak dname_cname_can_synth returns true if the synthesized dname is longer than allowed and therefore cannot be synthesized. The description is correct, but the function name indicates reverse meaning. mvavrusa: right, fixed in c4770131

jvcelak: function put_answer() does not differentiate between UDP and TCP. If ANY requests over UDP are prohibited, also TCP ANY requests will be rejected. mvavrusa: I reworked the processing flags, see fc08b7c0 for code and commit message jvcelak: Much, much, much better.

jvcelak: put_rr(), put_authority_soa(): if knot_pkt_put() fails due to limited packet size, the copied RR set will leak. mvavrusa: good catch! there was another one, see 5cc27092 jvcelak:

jvcelak: ixfr_answer_soa should be declared static and removed from the module interface, it is called only from ixfr_answer jvcelak:

jvcelak: discussed with @mvavrusa: TRUNC does not necessarily mean that the packet has TC flag set, although the description of the constant says so. Maybe the processing control can be improved in this area.

jvcelak: Can ns_proc_query_reset function be written less cryptically? Why do not call the destructors, clear the whole structure, and reset just the preserved fields? mvavrusa: can you check 42e36fc5 ? jvcelak: looks better

jvcelak: The comment in ns_proc_query_out says that positive answers are restricted. IMHO all answers are restricted. The same with transaction security... Remove the comments? mvavrusa:

jvcelak: in update_answer, should not we check permissions before trying to lock the update mutex? We can disclose that some update is already in progress, even if the request is not authenticated. mvavrusa: well spotted fixed in c64db7

Finally. The first review is finished. I have checked everything except the NSEC3 proves (src/libknot/nameserver/nsec_proofs), which are a subject of a next refactoring #191 (closed)

To sum it up, the merge request looks pretty good and can be merged as soon as the opened questions are resolved.

Jenkins is fixed. But the unit tests are still failing on OpenBSD: ns......................FAILED 35 mvavrusa: fixed in 3589c7 jvcelak:

mentioned in issue #197 (closed)

removed milestone