EDNS: Fixed packet handling of OPT

src/knot/nameserver/internet.c

@@ -655,7 +655,7 @@ static int solve_additional(int state, knot_pkt_t *pkt,
                             struct query_data *qdata, void *ctx)
 {
 	/* Put OPT RR. */
-	int ret = knot_pkt_put_opt(pkt);
+	int ret = knot_pkt_write_opt(pkt);
 
 	/* Scan all RRs in ANSWER/AUTHORITY. */
 	for (uint16_t i = 0; i < pkt->rrset_count; ++i) {

src/knot/nameserver/process_query.c

@@ -534,7 +534,7 @@ static int prepare_answer(const knot_pkt_t *query, knot_pkt_t *resp, knot_proces
 
 	knot_rrset_t *opt_rr = knot_edns_new_from_params(server->edns,
 	                                             knot_pkt_have_nsid(query),
-	                                             mm); /* TODO-REVIEW: ok to use memory context? */
+	                                             &ctx->mm); /* TODO-REVIEW: ok to use memory context? */
 	if (opt_rr == NULL) {
 		dbg_ns("%s: can't create OPT RR (%d)\n", __func__, ret);
 		return ret;
@@ -551,23 +551,18 @@ static int prepare_answer(const knot_pkt_t *query, knot_pkt_t *resp, knot_proces
 		knot_edns_set_do(resp->opt_rr);
 	}
 
-	/* Set minimal supported size from EDNS(0). */
+	/* Get minimal supported size from EDNS(0). */
 	uint16_t client_maxlen = knot_edns_get_payload(query->opt_rr);
 	uint16_t server_maxlen = knot_edns_get_payload(resp->opt_rr);
-
-	/*! \warning [OPT] This is wrong: we should always advertise the
-	 *           server's max allowed payload. This minimum only applies for
-	 *           this response.
-	 */
-	/*! \todo [OPT] REWRITE */
-//	resp->opt_rr.payload = MIN(client_maxlen, server_maxlen);
+	uint16_t min_edns = MIN(client_maxlen, server_maxlen);
 
 	/* Update packet size limit. */
-	/*! \todo [OPT] REWRITE */
-//	if (qdata->param->proc_flags & NS_QUERY_LIMIT_SIZE) {
-//		resp->max_size =  MAX(resp->max_size, resp->opt_rr.payload);
-//		dbg_ns("%s: packet size limit <= %zuB\n", __func__, resp->max_size);
-//	}
+	if (qdata->param->proc_flags & NS_QUERY_LIMIT_SIZE) {
+		resp->max_size =  MAX(resp->max_size, min_edns);
+		dbg_ns("%s: packet size limit <= %zuB\n", __func__, resp->max_size);
+	}
+
+	/* In the response, always advertise its maximum UPD payload size.*/
 
 	return ret;
 }

src/knot/server/server.c

@@ -463,7 +463,7 @@ static int edns_reconfigure(const struct conf_t *conf, server_t *server)
 	knot_edns_params_t *edns =
 		knot_edns_new_params(conf->max_udp_payload, KNOT_EDNS_VERSION,
 	                             KNOT_EDNS_DEFAULT_FLAGS, conf->nsid_len,
-	                             conf->nsid);
+	                             (uint8_t *)conf->nsid);
 	if (edns == NULL) {
 		log_server_error("Couldn't initialize EDNS(0), please restart.\n");
 	}

src/libknot/packet/pkt.c

@@ -808,50 +808,8 @@ int knot_pkt_add_opt(knot_pkt_t *pkt, knot_rrset_t *opt_rr)
 
 	pkt->opt_rr = opt_rr;
 
-//	// copy the OPT RR
-
-//	/*! \todo Change the way OPT RR is handled in response.
-//	 *        Pointer to nameserver->opt_rr should be enough.
-//	 */
-
-//	resp->opt_rr.version = opt_rr->version;
-//	resp->opt_rr.ext_rcode = opt_rr->ext_rcode;
-//	resp->opt_rr.payload = opt_rr->payload;
-
-//	/*
-//	 * Add options only if NSID is requested.
-//	 *
-//	 * This is a bit hack and should be resolved in other way before some
-//	 * other options are supported.
-//	 */
-
-//	if (add_nsid && opt_rr->option_count > 0) {
-//		resp->opt_rr.option_count = opt_rr->option_count;
-//		assert(resp->opt_rr.options == NULL);
-//		resp->opt_rr.options = (knot_opt_option_t *)malloc(
-//				 resp->opt_rr.option_count * sizeof(knot_opt_option_t));
-//		CHECK_ALLOC_LOG(resp->opt_rr.options, KNOT_ENOMEM);
-
-//		memcpy(resp->opt_rr.options, opt_rr->options,
-//		       resp->opt_rr.option_count * sizeof(knot_opt_option_t));
-
-//		// copy all data
-//		for (int i = 0; i < opt_rr->option_count; i++) {
-//			resp->opt_rr.options[i].data = (uint8_t *)malloc(
-//						resp->opt_rr.options[i].length);
-//			CHECK_ALLOC_LOG(resp->opt_rr.options[i].data, KNOT_ENOMEM);
-
-//			memcpy(resp->opt_rr.options[i].data,
-//			       opt_rr->options[i].data,
-//			       resp->opt_rr.options[i].length);
-//		}
-//		resp->opt_rr.size = opt_rr->size;
-//	} else {
-//		resp->opt_rr.size = EDNS_MIN_SIZE;
-//	}
-
-	/* OPT RR is not directly converted to wire, thus we must reserve space
-	 * for it.
+	/* OPT RR is not directly converted to wire, we must wait till the
+	 * Additional section is started. Now just reserve space for it.
 	 */
 	pkt->reserved += knot_edns_size(opt_rr);
 
@@ -860,7 +818,7 @@ int knot_pkt_add_opt(knot_pkt_t *pkt, knot_rrset_t *opt_rr)
 
 /*----------------------------------------------------------------------------*/
 
-int knot_pkt_put_opt(knot_pkt_t *pkt)
+int knot_pkt_write_opt(knot_pkt_t *pkt)
 {
 	if (pkt == NULL) {
 		return KNOT_EINVAL;
@@ -894,22 +852,10 @@ int knot_pkt_put_opt(knot_pkt_t *pkt)
 		pkt->size += len;
 		pkt_rr_wirecount_add(pkt, pkt->current, rr_added);
 		pkt->reserved -= ret;
-	}
-
-
-//	int ret = knot_edns_to_wire(pkt->opt_rr,
-//	                            pkt->wire + pkt->size,
-//	                            pkt->max_size - pkt->size);
-//	if (ret <= 0) {
-//		return ret;
-//	}
 
-//	pkt_rr_wirecount_add(pkt, pkt->current, 1);
-//	pkt->size += ret;
-//	pkt->reserved -= ret;
-
-	dbg_packet("%s: OPT RR written, new packet size %zu\n", __func__,
-	           pkt->size);
+		dbg_packet("%s: OPT RR written, new packet size %zu\n",
+		           __func__, pkt->size);
+	}
 
 	return KNOT_EOK;
 }

src/libknot/packet/pkt.h

@@ -236,22 +236,18 @@ const knot_pktsection_t *knot_pkt_section(const knot_pkt_t *pkt,
  * EDNS(0)-related API
  */
 
-/*** <<< #190 DEPRECATED */
-/*----------------------------------------------------------------------------*/
 /*!
- * \brief Sets the OPT RR of the response.
+ * \brief Stores OPT RR to the packet.
  *
- * This function also allocates space for the wireformat of the response, if
- * the payload in the OPT RR is larger than the current maximum size of the
- * response and copies the current wireformat over to the new space.
+ * The given OPT RR is used (i.e. pointer to it is stored in the packet). Be
+ * careful not to free the OPT RR somewhere else. In fact, it will be freed
+ * by the packet structure.
  *
- * \note The contents of the OPT RR are copied.
+ * \warning This function does not write the OPT RR to wireformat of the packet.
+ *          To do that, call knot_pkt_write_opt() after this function.
  *
- * \note It is expected that resp.max_size is already set to correct value as
- *       it is impossible to distinguish TCP scenario in this function.
- *
- * \param resp Response to set the OPT RR to.
- * \param opt_rr OPT RR to set.
+ * \param pkt     Packet to set the OPT RR to.
+ * \param opt_rr  OPT RR to set.
  *
  * \retval KNOT_EOK
  * \retval KNOT_EINVAL
@@ -261,13 +257,28 @@ const knot_pktsection_t *knot_pkt_section(const knot_pkt_t *pkt,
  */
 int knot_pkt_add_opt(knot_pkt_t *pkt, knot_rrset_t *opt_rr);
 
-/*----------------------------------------------------------------------------*/
-/*** >>> #190 DEPRECATED */
-
-/*! \brief Write OPT RR to wireformat.
- *  \note Legacy API.
+/*!
+ * \brief Write OPT RR to wireformat.
+ *
+ * If used properly (i.e. the OPT RR was added by knot_pkt_add_opt()), there
+ * should be space reserved for the OPT RR, so it does not matter when during
+ * Additional processing this function is called. (Well, it should not be used
+ * after signing the packet with TSIG, of course.)
+ *
+ * \note This function should be called only after the Additional section of
+ *       packet was started.
+ *
+ * \param pkt Packet in which the OPT RR should be written.
+ *
+ * \retval KNOT_EOK if successful.
+ * \retval KNOT_EINVAL if the parameter is NULL.
+ * \retval KNOT_ENOTSUP if the Additional section of the packet was not yet
+ *                      started.
+ * \retval KNOT_ESPACE if the RR did not fit in. (Should not happen as the
+ *                     packet should have reserved space for the OPT RR when it
+ *                     was added.
  */
-int knot_pkt_put_opt(knot_pkt_t *pkt);
+int knot_pkt_write_opt(knot_pkt_t *pkt);
 
 
 /*

src/utils/dig/dig_exec.c

@@ -157,7 +157,7 @@ static knot_pkt_t* create_query_packet(const query_t *query)
 
 		// Write prepared OPT to wire
 		knot_pkt_begin(packet, KNOT_ADDITIONAL);
-		ret |= knot_pkt_put_opt(packet);
+		ret |= knot_pkt_write_opt(packet);
 
 		if (ret != KNOT_EOK) {
 			ERR("can't set up EDNS section\n");

tests/pkt.c

@@ -135,7 +135,7 @@ int main(int argc, char *argv[])
 	ok(ret == KNOT_EOK, "pkt: begin ADDITIONALS");
 
 	/* Encode OPT RR. */
-	ret = knot_pkt_put_opt(out);
+	ret = knot_pkt_write_opt(out);
 	ok(ret == KNOT_EOK, "pkt: write OPT RR");
 
 	/*