Fix first batch of review comments by Wouter

Author: k0ekk0ek

compat/getopt.h

@@ -1,5 +1,5 @@
 /*
- * getopt.h -- some useful comment
+ * getopt.h -- getopt definitions for platform that are missing unistd.h
  *
  * Copyright (c) 2023, NLnet Labs. All rights reserved.
  *

doc/manual/design_notes.rst

@@ -61,6 +61,22 @@ therefore managed on a per file base.
 On-the-fly indexing is a good tradeoff, but does introduce some complexity
 with regards to handling partial tokens.
 
+Padded operation
+^^^^^^^^^^^^^^^^
+
+The AVX-512 instruction set offers masked load and store operations, earlier
+instruction sets, like SSE4.2 and AVX2 do not. |project| requires input
+buffers to be sufficiently large enough to ensure optimized operations can
+safely load blocks of input data without reading past the buffer limit. This
+requirement is of no concern to the user when parsing files as |project|
+manages input buffers. The requirement is of concern when the user provides
+the input directly as the buffer must be null-terminated and padded.
+
+.. note::
+   Future releases may lift this requirement by using fallback
+   implementations for the last set of tokens that are not sufficiently
+   padded.
+
 
 Comments
 --------

include/zone/attributes.h

@@ -28,17 +28,6 @@
 # define zone_attribute(params)
 #endif
 
-#if defined __has_attribute
-# define zone_has_attribute(params) __has_attribute(params)
-# define zone_attribute(params) __attribute__(params)
-#elif zone_gcc
-# define zone_has_attribute(params) __has_attribute(params)
-# define zone_attribute(params) __attribute__(params)
-#else
-# define zone_has_attribute(params)
-# define zone_attribute(params)
-#endif
-
 #define zone_nonnull(params) zone_attribute((__nonnull__ params))
 #define zone_nonnull_all zone_attribute((__nonnull__))
 

src/fallback/parser.c

@@ -35,7 +35,7 @@
 #include "generic/apl.h"
 #include "generic/svcb.h"
 #include "generic/cert.h"
-#include "generic/dnssec.h"
+#include "generic/algorithm.h"
 #include "generic/types.h"
 #include "generic/type.h"
 #include "generic/format.h"

src/generic/algorithm.h

@@ -1,13 +1,13 @@
 /**
- * dnssec.h
+ * algorithm.h -- Algorithm RDATA parser
  *
  * Copyright (c) 2023, NLnet Labs. All rights reserved.
  *
  * SPDX-License-Identifier: BSD-3-Clause
  *
  */
-#ifndef DNSSEC_H
-#define DNSSEC_H
+#ifndef ALGORITHM_H
+#define ALGORITHM_H
 
 // https://www.iana.org/assignments/dns-sec-alg-numbers/dns-sec-alg-numbers.xhtml
 
@@ -109,12 +109,8 @@ static uint8_t algorithm_hash(uint64_t value)
 
 nonnull_all
 warn_unused_result
-static really_inline int32_t parse_algorithm_type(
-  parser_t *parser,
-  const type_info_t *type,
-  const rdata_info_t *field,
-  rdata_t *rdata,
-  const token_t *token)
+static really_inline int32_t scan_algorithm(
+  const char *data, size_t length, uint8_t *number)
 {
   static const int8_t zero_masks[48] = {
     -1, -1, -1, -1, -1, -1, -1, -1,
@@ -125,17 +121,15 @@ static really_inline int32_t parse_algorithm_type(
      0,  0,  0,  0,  0,  0,  0,  0
   };
 
-  uint32_t number = (uint8_t)token->data[0] - '0';
-
-  if (unlikely(number > 9)) {
+  if ((uint8_t)*data - '0' > 9) {
     uint64_t input;
-    memcpy(&input, token->data, 8);
+    memcpy(&input, data, 8);
     const uint64_t letter_mask = 0x4040404040404040llu;
     // convert to upper case
-    input &= input & ~((input & letter_mask) >> 1);
+    input &= ~((input & letter_mask) >> 1);
     // zero out non-relevant bytes
     uint64_t zero_mask;
-    memcpy(&zero_mask, &zero_masks[32 - (token->length & 0x1f)], 8);
+    memcpy(&zero_mask, &zero_masks[32 - (length & 0x1f)], 8);
     input &= zero_mask;
     const uint8_t index = algorithm_hash(input);
     assert(index < 16);
@@ -145,36 +139,35 @@ static really_inline int32_t parse_algorithm_type(
     memcpy(&name, algorithm->key.name, 8);
     matches = input == name;
     // compare bytes 8-15
-    memcpy(&input, token->data + 8, 8);
+    memcpy(&input, data + 8, 8);
     memcpy(&mask, algorithm_hash_map[index].mask + 8, 8);
     memcpy(&name, algorithm->key.name + 8, 8);
     matches &= (input & mask) == name;
     // compare bytes 16-23
-    memcpy(&input, token->data + 16, 8);
+    memcpy(&input, data + 16, 8);
     memcpy(&mask, algorithm_hash_map[index].mask + 16, 8);
     memcpy(&name, algorithm->key.name + 16, 8);
     matches &= (input & mask) == name;
-    if (!(matches && (token->length == algorithm->key.length) && number > 0))
-      SYNTAX_ERROR(parser, "Invalid %s in %s", NAME(field), NAME(type));
-    *rdata->octets++ = (uint8_t)algorithm->value;
-    return 0;
+    *number = algorithm->value;
+    return matches & (length == algorithm->key.length) & (*number > 0);
   }
 
-  bool leading_zero = (number == 0) & (token->length > 1);
-  size_t length = 1;
-
-  for (; length < token->length; length++) {
-    const uint8_t digit = (uint8_t)token->data[length] - '0';
-    if (digit > 9)
-      break;
-    number = number * 10 + digit;
-  }
+  return scan_int8(data, length, number);
+}
 
-  *rdata->octets++ = (uint8_t)number;
-  if (number > 255 || length > 3 || length != token->length || leading_zero)
+nonnull_all
+warn_unused_result
+static really_inline int32_t parse_algorithm(
+  parser_t *parser,
+  const type_info_t *type,
+  const rdata_info_t *field,
+  rdata_t *rdata,
+  const token_t *token)
+{
+  if (!scan_algorithm(token->data, token->length, rdata->octets))
     SYNTAX_ERROR(parser, "Invalid %s in %s", NAME(field), NAME(type));
-
+  rdata->octets++;
   return 0;
 }
 
-#endif // DNSSEC_H
+#endif // ALGORITHM_H

src/generic/cert.h

@@ -67,12 +67,8 @@ static uint8_t certificate_hash(uint64_t value)
 }
 
 nonnull_all
-static really_inline int32_t parse_certificate_type(
-  parser_t *parser,
-  const type_info_t *type,
-  const rdata_info_t *field,
-  rdata_t *rdata,
-  const token_t *token)
+static really_inline int32_t scan_certificate_type(
+  const char *data, size_t length, uint16_t *type)
 {
   static const int8_t zero_masks[48] = {
     -1, -1, -1, -1, -1, -1, -1, -1,
@@ -83,49 +79,43 @@ static really_inline int32_t parse_certificate_type(
      0,  0,  0,  0,  0,  0,  0,  0
   };
 
-  uint32_t number = (uint8_t)token->data[0] - '0';
-
-  if (number > 9) {
+  if ((uint8_t)*data - '0' > 9) {
     uint64_t input;
-    memcpy(&input, token->data, 8);
+    memcpy(&input, data, 8);
     static const uint64_t letter_mask = 0x4040404040404040llu;
-    const size_t length = token->length;
     // convert to upper case
-    input &= input & ~((input & letter_mask) >> 1);
+    input &= ~((input & letter_mask) >> 1);
     // zero out non-relevant bytes
     uint64_t zero_mask;
-    memcpy(&zero_mask, &zero_masks[32 - (token->length & 0xf)], 8);
+    memcpy(&zero_mask, &zero_masks[32 - (length & 0xf)], 8);
     input &= zero_mask;
     const uint8_t index = certificate_hash(input);
     assert(index < 16);
     const certificate_type_t *certificate_type = certificate_type_map[index];
     uint64_t name;
     memcpy(&name, certificate_type->key.name, 8);
-    uint16_t value = certificate_type->value;
-    if (unlikely((input != name) & (length != certificate_type->key.length) & (value != 0)))
-      SYNTAX_ERROR(parser, "Invalid %s in %s", NAME(field), NAME(type));
-    value = htobe16(value);
-    memcpy(rdata->octets, &value, 2);
-    rdata->octets += 2;
-    return 0;
-  }
-
-  bool leading_zero = (number == 0) & (token->length > 1);
-  size_t length = 1;
-
-  for (; length < token->length; length++) {
-    const uint8_t digit = (uint8_t)token->data[length] - '0';
-    if (digit > 9)
-      break;
-    number = number * 10 + digit;
+    *type = certificate_type->value;
+    return (input == name) &
+      (length == certificate_type->key.length) &
+      (*type != 0);
   }
 
-  if (number > 65535 || length > 5 || length != token->length || leading_zero)
-    SYNTAX_ERROR(parser, "Invalid %s in %s", NAME(field), NAME(type));
+  return scan_int16(data, length, type);
+}
 
-  uint16_t value = (uint16_t)number;
-  value = htobe16(value);
-  memcpy(rdata->octets, &value, 2);
+nonnull_all
+static really_inline int32_t parse_certificate_type(
+  parser_t *parser,
+  const type_info_t *type,
+  const rdata_info_t *field,
+  rdata_t *rdata,
+  const token_t *token)
+{
+  uint16_t cert;
+  if (!scan_certificate_type(token->data, token->length, &cert))
+      SYNTAX_ERROR(parser, "Invalid %s in %s", NAME(field), NAME(type));
+  cert = htobe16(cert);
+  memcpy(rdata->octets, &cert, 2);
   rdata->octets += 2;
   return 0;
 }

src/generic/types.h

@@ -1270,7 +1270,7 @@ static int32_t parse_cert_rdata(
     return code;
   if ((code = take_contiguous(parser, type, &fields[2], token)) < 0)
     return code;
-  if ((code = parse_algorithm_type(parser, type, &fields[2], rdata, token)) < 0)
+  if ((code = parse_algorithm(parser, type, &fields[2], rdata, token)) < 0)
     return code;
   take(parser, token);
   if ((code = parse_base64_sequence(parser, type, &fields[3], rdata, token)) < 0)
@@ -1346,7 +1346,7 @@ static int32_t parse_ds_rdata(
     return code;
   if ((code = take_contiguous(parser, type, &fields[1], token)) < 0)
     return code;
-  if ((code = parse_algorithm_type(parser, type, &fields[1], rdata, token)) < 0)
+  if ((code = parse_algorithm(parser, type, &fields[1], rdata, token)) < 0)
     return code;
   if ((code = take_contiguous(parser, type, &fields[2], token)) < 0)
     return code;
@@ -1375,7 +1375,7 @@ static int32_t check_sshfp_rr(
 
   // https://www.iana.org/assignments/dns-sshfp-rr-parameters
 
-  if (c >= n)
+  if (c == n)
     SYNTAX_ERROR(parser, "Missing %s in %s", NAME((&f[0])), NAME(type));
   else if (o[1] == 1 && (n - c) != 20)
     SEMANTIC_ERROR(parser, "Wrong fingerprint size for type %s in %s",
@@ -1398,15 +1398,28 @@ static int32_t parse_sshfp_rdata(
     return code;
   if ((code = parse_int8(parser, type, &fields[0], rdata, token)) < 0)
     return code;
+
+  const uint8_t *fingerprint_type = rdata->octets;
   if ((code = take_contiguous(parser, type, &fields[1], token)) < 0)
     return code;
   if ((code = parse_int8(parser, type, &fields[1], rdata, token)) < 0)
     return code;
+
+  const uint8_t *fingerprint = rdata->octets;
   take(parser, token);
   if ((code = parse_base16_sequence(parser, type, &fields[2], rdata, token)) < 0)
     return code;
 
-  return check_sshfp_rr(parser, type, rdata);
+  // https://www.iana.org/assignments/dns-sshfp-rr-parameters
+  size_t fingerprint_size = (uintptr_t)rdata->octets - (uintptr_t)fingerprint;
+  if (unlikely(*fingerprint_type == 1 && fingerprint_size != 20))
+    SEMANTIC_ERROR(parser, "Wrong fingerprint size for type %s in %s",
+                           "SHA1", NAME(type));
+  if (unlikely(*fingerprint_type == 2 && fingerprint_size != 32))
+    SEMANTIC_ERROR(parser, "Wrong fingerprint size for type %s in %s",
+                           "SHA256", NAME(type));
+
+  return accept_rr(parser, type, rdata);
 }
 
 nonnull_all
@@ -1603,7 +1616,7 @@ static int32_t parse_rrsig_rdata(
     return code;
   if ((code = take_contiguous(parser, type, &fields[1], token)) < 0)
     return code;
-  if ((code = parse_algorithm_type(parser, type, &fields[1], rdata, token)) < 0)
+  if ((code = parse_algorithm(parser, type, &fields[1], rdata, token)) < 0)
     return code;
   if ((code = take_contiguous(parser, type, &fields[2], token)) < 0)
     return code;
@@ -1710,7 +1723,7 @@ static int32_t parse_dnskey_rdata(
     return code;
   if ((code = take_contiguous(parser, type, &fields[2], token)) < 0)
     return code;
-  if ((code = parse_algorithm_type(parser, type, &fields[2], rdata, token)) < 0)
+  if ((code = parse_algorithm(parser, type, &fields[2], rdata, token)) < 0)
     return code;
   take(parser, token);
   if ((code = parse_base64_sequence(parser, type, &fields[3], rdata, token)) < 0)

src/generic/wks.h

@@ -195,7 +195,7 @@ static really_inline uint8_t service_hash(uint64_t input, size_t length)
 }
 
 nonnull((1,4))
-static really_inline bool scan_service(
+static really_inline int32_t scan_service(
   const char *data, size_t length, int32_t protocol, uint16_t *port)
 {
   uint8_t digit = (uint8_t)*data - '0';
@@ -242,18 +242,7 @@ static really_inline bool scan_service(
 	   (services[index].key.length == length);
   }
 
-  size_t count = 1;
-  int32_t number = digit;
-
-  // FIXME: check for leading zero!
-  if (length > 5) // port numbers must be between 0 and 65535
-    return 0;
-  for (; ((digit = ((uint8_t)data[count] - '0')) <= 9); count++)
-    number = number * 10 + digit;
-  if (count != length || number > 65535)
-    return -1;
-  *port = (uint16_t)number;
-  return true;
+  return scan_int16(data, length, port);
 }
 
 #endif // WKS_H

src/haswell/parser.c

@@ -36,7 +36,7 @@
 #include "generic/apl.h"
 #include "generic/svcb.h"
 #include "generic/cert.h"
-#include "generic/dnssec.h"
+#include "generic/algorithm.h"
 #include "generic/types.h"
 #include "westmere/type.h"
 #include "generic/format.h"

src/westmere/bits.h

@@ -34,8 +34,6 @@ static inline uint64_t leading_zeroes(uint64_t input_num) {
 }
 
 static inline uint64_t prefix_xor(const uint64_t bitmask) {
-  // There should be no such thing with a processor supporting avx2
-  // but not clmul.
   __m128i all_ones = _mm_set1_epi8('\xFF');
   __m128i result = _mm_clmulepi64_si128(_mm_set_epi64x(0ULL, (long long)bitmask), all_ones, 0);
   return (uint64_t)_mm_cvtsi128_si64(result);

src/westmere/parser.c

@@ -36,7 +36,7 @@
 #include "generic/apl.h"
 #include "generic/svcb.h"
 #include "generic/cert.h"
-#include "generic/dnssec.h"
+#include "generic/algorithm.h"
 #include "generic/types.h"
 #include "westmere/type.h"
 #include "generic/format.h"

src/zone.c

@@ -15,7 +15,7 @@
 #include <stdlib.h>
 #include <limits.h>
 #if _WIN32
-# include <Windows.h>
+# include <windows.h>
 # include <shlwapi.h>
 #endif