Merge 5791f6ba3f into 9a37bb659b

src/datasets.h

@@ -15,81 +15,191 @@
  * 02110-1301, USA.
  */
 
+/**
+ * \file
+ *
+ * Datasets: named in-memory sets that rules match against with the
+ * 'dataset' and 'datarep' keywords.
+ *
+ * Each set holds one kind of value (string, MD5, SHA256, IPv4 or IP).
+ * Entries may carry a reputation value, used by 'datarep'.
+ *
+ * Return codes, unless a function says otherwise:
+ *    1  done (added, found, removed)
+ *    0  nothing to do (already present, not found, busy)
+ *   -1  error
+ *   -2  bad input (wrong length or unparseable text)
+ *
+ * Locking: one global, non-recursive lock (DatasetLock()). Only the
+ * functions marked "caller holds the lock" need you to take it.
+ */
+
 #ifndef SURICATA_DATASETS_H
 #define SURICATA_DATASETS_H
 
 // forward declaration to make things opaque to bindgen
+
+/** Reputation value stored with an entry. */
 typedef uint16_t DataRepType;
+
 typedef struct Dataset Dataset;
+
+/** \brief Add a value.
+ *  \retval 1 added, 0 already present, -1 error, -2 bad length */
 int SCDatasetAdd(Dataset *set, const uint8_t *data, const uint32_t data_len);
+
+/** \brief Add a value with a reputation. \p rep must not be NULL.
+ *  If the value already exists, its old reputation is kept.
+ *  \retval 1 added, 0 already present, -1 error, -2 bad length */
 int SCDatasetAddwRep(
         Dataset *set, const uint8_t *data, const uint32_t data_len, const DataRepType *rep);
 
 #ifndef SURICATA_BINDGEN_H
+
 #include "util-thash.h"
 #include "rust.h"
 #include "datasets-reputation.h"
 
+/** \name Lifecycle
+ *  @{ */
+
+/** \brief Create the sets declared in the 'datasets' YAML section.
+ *  Call once at startup. Fatal on bad config. */
 int DatasetsInit(void);
+
+/** \brief Free all sets. */
 void DatasetsDestroy(void);
+
+/** \brief Write each set that has a save path to that file. */
 void DatasetsSave(void);
+
+/** \brief Start a live reload: mark static sets (load file, no save
+ *  path, not from YAML) as hidden. They stay usable until
+ *  DatasetPostReloadCleanup(). */
 void DatasetReload(void);
+
+/** \brief Finish a live reload: free the hidden sets. */
 void DatasetPostReloadCleanup(void);
 
+/** @} */
+
+/** File format of a set's load/save file. */
 typedef enum {
-    DATASET_FORMAT_CSV = 0,
-    DATASET_FORMAT_JSON,   /* File contains one single JSON object */
-    DATASET_FORMAT_NDJSON, /* Newline Delimited JSON */
+    DATASET_FORMAT_CSV = 0, /**< CSV (default) */
+    DATASET_FORMAT_JSON,    /**< one JSON document */
+    DATASET_FORMAT_NDJSON,  /**< one JSON object per line */
 } DatasetFormats;
 
+/** Kind of value a set holds. */
 enum DatasetTypes {
-#define DATASET_TYPE_NOTSET 0
-    DATASET_TYPE_STRING = 1,
-    DATASET_TYPE_MD5,
-    DATASET_TYPE_SHA256,
-    DATASET_TYPE_IPV4,
-    DATASET_TYPE_IPV6,
+#define DATASET_TYPE_NOTSET 0   /** no type yet */
+    DATASET_TYPE_STRING = 1,    /**< any bytes */
+    DATASET_TYPE_MD5,           /**< 16 bytes */
+    DATASET_TYPE_SHA256,        /**< 32 bytes */
+    DATASET_TYPE_IPV4,          /**< 4 bytes */
+    DATASET_TYPE_IPV6,          /**< 16 bytes; IPv4 stored in the first 4, rest zero */
 };
 
+/** Max set name length, without the NUL. */
 #define DATASET_NAME_MAX_LEN 63
+
+/** A set. Sets are kept in a global linked list. */
 typedef struct Dataset {
-    char name[DATASET_NAME_MAX_LEN + 1];
-    enum DatasetTypes type;
-    uint32_t id;
-    bool from_yaml;                     /* Mark whether the set was retrieved from YAML */
-    bool hidden;                        /* Mark the old sets hidden in case of reload */
-    bool remove_key;                    /* Mark that value key should be removed from extra data */
-    THashTableContext *hash;
-
-    char load[PATH_MAX];
-    char save[PATH_MAX];
-
-    struct Dataset *next;
+    char name[DATASET_NAME_MAX_LEN + 1];/**< name */
+    enum DatasetTypes type;             /**< value type */
+    uint32_t id;                        /**< unique id */
+    bool from_yaml;                     /**< declared in suricata.yaml */
+    bool hidden;                        /**< replaced by a reload, pending cleanup */
+    bool remove_key;                    /**< strip value key from extra data */
+    THashTableContext *hash;            /**< the values */
+    char load[PATH_MAX];                /**< file read at startup, or "" */
+    char save[PATH_MAX];                /**< file written by DatasetsSave(), or "" */
+    struct Dataset *next;               /**< next set in the list */
 } Dataset;
 
+/** \brief Map "string", "md5", "sha256", "ipv4" or "ip" (case
+ *  insensitive) to a type. Anything else gives DATASET_TYPE_NOTSET. */
 enum DatasetTypes DatasetGetTypeFromString(const char *s);
+
+/** \brief Add a fully built set to the global list.
+ *  Caller holds the lock.
+ *  \retval 0 ok, -1 no hash or memcap reached (caller still owns the set) */
 int DatasetAppendSet(Dataset *set);
+
+/** \brief Allocate a zeroed set with a new id. Does not set the name. */
 Dataset *DatasetAlloc(const char *name);
+
+/** \brief Take the global lock. */
 void DatasetLock(void);
+
+/** \brief Release the global lock. */
 void DatasetUnlock(void);
+
+/** \brief Find a visible set by name (case insensitive), any type.
+ *  Caller holds the lock. \return the set or NULL */
 Dataset *DatasetSearchByName(const char *name);
+
+/** \brief Find a set by name. Never creates one.
+ *  \return the set, or NULL if missing or of another type */
 Dataset *DatasetFind(const char *name, enum DatasetTypes type);
+
+/** \brief Get a set, creating, loading and registering it if needed.
+ *  This is what rules use. Pass NULL/"" for \p save and \p load, and
+ *  0 for \p memcap and \p hashsize, to use defaults.
+ *  \return the set, or NULL on error (e.g. type mismatch) */
 Dataset *DatasetGet(const char *name, enum DatasetTypes type, const char *save, const char *load,
         uint64_t memcap, uint32_t hashsize);
+
+/** \brief Building block of DatasetGet(): return an existing set, or
+ *  allocate a new one with no hash and not in the list.
+ *  Fills in default \p memcap / \p hashsize when they are 0.
+ *  Caller holds the lock.
+ *  \retval 1 existing set, 0 new set, -1 error */
 int DatasetGetOrCreate(const char *name, enum DatasetTypes type, const char *save, const char *load,
         uint64_t *memcap, uint32_t *hashsize, Dataset **ret_set);
+
+/** \brief Remove a value.
+ *  \retval 1 removed, 0 in use (retry later), -1 not found or error,
+ *          -2 bad length */
 int DatasetRemove(Dataset *set, const uint8_t *data, const uint32_t data_len);
+
+/** \name Lookup
+ *  @{ */
+
+/** \brief Is the value in the set?
+ *  \retval 1 found, 0 not found, -1 error or bad length */
 int DatasetLookup(Dataset *set, const uint8_t *data, const uint32_t data_len);
+
+/** \brief Look up a value and get its stored reputation in .rep.
+ *  \p rep must not be NULL. Not found and error both give
+ *  .found = false. */
 DataRepResultType DatasetLookupwRep(Dataset *set, const uint8_t *data, const uint32_t data_len,
         const DataRepType *rep);
 
+/** @} */
+
+/** \brief Default memcap and hash size, with 'datasets.defaults'
+ *  from YAML applied. Hash size defaults to 4096. */
 void DatasetGetDefaultMemcap(uint64_t *memcap, uint32_t *hashsize);
+
+/** \brief Parse an IPv4 or IPv6 address into the 16-byte IP format.
+ *  IPv4 (and IPv4-mapped IPv6) go in the first 4 bytes, rest zero.
+ *  Fatal at startup on bad input. \retval 0 ok, -1 bad address */
 int DatasetParseIpv6String(Dataset *set, const char *line, struct in6_addr *in6);
 
+/** \name Text variants
+ *
+ *  Same as add/lookup/remove, but the value is text: base64 for
+ *  strings, hex for hashes, normal notation for IPs.
+ *  Return -2 if the text can't be parsed.
+ *  @{ */
+
 int DatasetAddSerialized(Dataset *set, const char *string);
 int DatasetRemoveSerialized(Dataset *set, const char *string);
 int DatasetLookupSerialized(Dataset *set, const char *string);
 
+/** @} */
+
 #endif // SURICATA_BINDGEN_H
 
 #endif /* SURICATA_DATASETS_H */

src/decode-teredo.h

@@ -15,12 +15,42 @@
  * 02110-1301, USA.
  */
 
+/**
+ * \file
+ *
+ * Teredo decoder (RFC 4380): IPv6 tunneled inside UDP.
+ *
+ * Teredo has no magic number, so detection is a best guess: the UDP
+ * payload must look like a sane IPv6 packet. Port filtering is done
+ * by the caller with DecodeTeredoEnabledForPort().
+ *
+ * Config (suricata.yaml):
+ *   decoder.teredo.enabled  default: yes
+ *   decoder.teredo.ports    default: any; at most 4 single ports
+ */
+
 #ifndef SURICATA_DECODE_TEREDO_H
 #define SURICATA_DECODE_TEREDO_H
 
+/** \brief Try to decode a UDP payload as Teredo.
+ *
+ *  Skips an optional origin indication, checks that an IPv6 packet
+ *  fills the rest of the payload, and queues it as a tunnel packet.
+ *
+ *  \param pkt UDP payload
+ *  \param len payload length
+ *  \retval TM_ECODE_OK      Teredo; tunnel packet queued
+ *  \retval TM_ECODE_FAILED  not Teredo, decoder disabled, or no
+ *                           tunnel packet could be created */
 int DecodeTeredo(ThreadVars *tv, DecodeThreadVars *dtv, Packet *p,
                  const uint8_t *pkt, uint16_t len);
+
+/** \brief Read the `decoder.teredo` settings. Call once at startup. */
 void DecodeTeredoConfig(void);
+
+/** \brief Should this UDP flow be tried as Teredo?
+ *  \return true if the decoder is enabled and either no ports are
+ *          configured ("any") or \p sp or \p dp is a configured port */
 bool DecodeTeredoEnabledForPort(const uint16_t sp, const uint16_t dp);
 
 #endif

src/defrag-hash.c

@@ -15,6 +15,19 @@
  * 02110-1301, USA.
  */
 
+/**
+ * \file
+ *
+ * Defrag tracker hash: one tracker per datagram being reassembled.
+ *
+ * New trackers come from the spare pool, else a fresh allocation, else
+ * (at memcap) an idle tracker evicted from the hash. Lookups return the
+ * tracker locked with its use count raised; DefragTrackerRelease()
+ * undoes both. A tracker with use_cnt > 0 is never evicted.
+ *
+ * Lock order: bucket lock, then tracker lock.
+ */
+
 #include "suricata-common.h"
 #include "conf.h"
 #include "defrag-hash.h"
@@ -30,20 +43,21 @@
 /** defrag tracker hash table */
 DefragTrackerHashRow *defragtracker_hash;
 DefragConfig defrag_config;
-SC_ATOMIC_DECLARE(uint64_t,defrag_memuse);
-SC_ATOMIC_DECLARE(unsigned int,defragtracker_counter);
-SC_ATOMIC_DECLARE(unsigned int,defragtracker_prune_idx);
+SC_ATOMIC_DECLARE(uint64_t,defrag_memuse);              /**< bytes in use */
+SC_ATOMIC_DECLARE(unsigned int,defragtracker_counter);  /**< trackers in use */
+SC_ATOMIC_DECLARE(unsigned int,defragtracker_prune_idx);/**< bucket where eviction resumes */
 
 static DefragTracker *DefragTrackerGetUsedDefragTracker(
         ThreadVars *tv, const DecodeThreadVars *dtv);
 
-/** queue with spare tracker */
+/** spare trackers, ready for reuse */
 static DefragTrackerStack defragtracker_spare_q;
 
 /**
- *  \brief Update memcap value
+ *  \brief Set a new memcap.
  *
- *  \param size new memcap value
+ *  \param size new memcap in bytes; must be above current memuse
+ *  \retval 1 set, 0 rejected
  */
 int DefragTrackerSetMemcap(uint64_t size)
 {
@@ -56,9 +70,8 @@ int DefragTrackerSetMemcap(uint64_t size)
 }
 
 /**
- *  \brief Return memcap value
- *
- *  \retval memcap value
+ *  \brief Get the memcap.
+ *  \retval memcap in bytes
  */
 uint64_t DefragTrackerGetMemcap(void)
 {
@@ -67,9 +80,8 @@ uint64_t DefragTrackerGetMemcap(void)
 }
 
 /**
- *  \brief Return memuse value
- *
- *  \retval memuse value
+ *  \brief Get current memory use.
+ *  \retval bytes in use (hash table, trackers and fragments)
  */
 uint64_t DefragTrackerGetMemuse(void)
 {
@@ -77,17 +89,21 @@ uint64_t DefragTrackerGetMemuse(void)
     return memusecopy;
 }
 
+/** \brief Policy applied to a packet when no tracker can be had. */
 enum ExceptionPolicy DefragGetMemcapExceptionPolicy(void)
 {
     return defrag_config.memcap_policy;
 }
 
+/** \brief Return a tracker to the spare pool.
+ *  It must already be unlinked from the hash and cleared. */
 void DefragTrackerMoveToSpare(DefragTracker *h)
 {
     DefragTrackerEnqueue(&defragtracker_spare_q, h);
     (void) SC_ATOMIC_SUB(defragtracker_counter, 1);
 }
 
+/** \return new *unlocked* tracker, or NULL if over memcap or out of memory */
 static DefragTracker *DefragTrackerAlloc(void)
 {
     if (!(DEFRAG_CHECK_MEMCAP(sizeof(DefragTracker)))) {
@@ -106,6 +122,7 @@ static DefragTracker *DefragTrackerAlloc(void)
     return dt;
 }
 
+/** \brief Free a tracker and its fragments, updating memuse. */
 static void DefragTrackerFree(DefragTracker *dt)
 {
     if (dt != NULL) {
@@ -117,14 +134,17 @@ static void DefragTrackerFree(DefragTracker *dt)
     }
 }
 
+/* use_cnt: number of packets currently holding the tracker */
 #define DefragTrackerIncrUsecnt(dt) \
     SC_ATOMIC_ADD((dt)->use_cnt, 1)
 #define DefragTrackerDecrUsecnt(dt) \
     SC_ATOMIC_SUB((dt)->use_cnt, 1)
 
+/** \brief Set up a tracker for the datagram \p p belongs to, and mark
+ *  it in use. */
 static void DefragTrackerInit(DefragTracker *dt, Packet *p)
 {
-    /* copy address */
+    /* copy addresses */
     COPY_ADDRESS(&p->src, &dt->src_addr);
     COPY_ADDRESS(&p->dst, &dt->dst_addr);
 
@@ -147,22 +167,28 @@ static void DefragTrackerInit(DefragTracker *dt, Packet *p)
     (void) DefragTrackerIncrUsecnt(dt);
 }
 
+/** \brief Give back a tracker obtained from a lookup: drop the use
+ *  count and unlock it. */
 void DefragTrackerRelease(DefragTracker *t)
 {
     (void) DefragTrackerDecrUsecnt(t);
     SCMutexUnlock(&t->lock);
 }
 
+/** \brief Free the fragments stored in the tracker. */
 void DefragTrackerClearMemory(DefragTracker *dt)
 {
     DefragTrackerFreeFrags(dt);
 }
 
+/* defaults, overridable in the `defrag` YAML section */
 #define DEFRAG_DEFAULT_HASHSIZE 4096
 #define DEFRAG_DEFAULT_MEMCAP 16777216
 #define DEFRAG_DEFAULT_PREALLOC 1000
 
-/** \brief initialize the configuration
+/** \brief Read the 'defrag' config, allocate the hash and, if
+ *  'defrag.prealloc' is true, fill the spare pool.
+ *  Exits if the memcap is invalid or too small for the hash/prealloc.
  *  \warning Not thread safe */
 void DefragInitConfig(bool quiet)
 {
@@ -182,12 +208,13 @@ void DefragInitConfig(bool quiet)
     SC_ATOMIC_SET(defrag_config.memcap, DEFRAG_DEFAULT_MEMCAP);
     defrag_config.memcap_policy = ExceptionPolicyParse("defrag.memcap-policy", false);
 
-    /* Check if we have memcap and hash_size defined at config */
+    /* override defaults with values from the config, if any */
     const char *conf_val;
     uint32_t configval = 0;
 
     uint64_t defrag_memcap;
-    /** set config values for memcap, prealloc and hash_size */
+    /** memcap (fatal if invalid), hash-size and trackers (warn and keep
+     *  default if invalid) */
     if ((SCConfGetNonNull("defrag.memcap", &conf_val)) == 1) {
         if (ParseSizeStringU64(conf_val, &defrag_memcap) < 0) {
             SCLogError("Error parsing defrag.memcap "
@@ -219,7 +246,7 @@ void DefragInitConfig(bool quiet)
                "%"PRIu32", prealloc: %"PRIu32, SC_ATOMIC_GET(defrag_config.memcap),
                defrag_config.hash_size, defrag_config.prealloc);
 
-    /* alloc hash memory */
+    /* allocate the hash table; it counts toward the memcap */
     uint64_t hash_size = defrag_config.hash_size * sizeof(DefragTrackerHashRow);
     if (!(DEFRAG_CHECK_MEMCAP(hash_size))) {
         SCLogError("allocating defrag hash failed: "
@@ -252,7 +279,7 @@ void DefragInitConfig(bool quiet)
 
     if ((SCConfGetNonNull("defrag.prealloc", &conf_val)) == 1) {
         if (SCConfValIsTrue(conf_val)) {
-            /* pre allocate defrag trackers */
+            /* preallocate 'defrag.trackers' trackers into the spare pool */
             for (i = 0; i < defrag_config.prealloc; i++) {
                 if (!(DEFRAG_CHECK_MEMCAP(sizeof(DefragTracker)))) {
                     SCLogError("preallocating defrag trackers failed: "
@@ -285,19 +312,20 @@ void DefragInitConfig(bool quiet)
     }
 }
 
-/** \brief shutdown the flow engine
+/** \brief Shut down the defrag hash: free the spare pool, every tracker
+ *  in the hash, and the hash itself.
  *  \warning Not thread safe */
 void DefragHashShutdown(void)
 {
     DefragTracker *dt;
 
-    /* free spare queue */
+    /* free the spare pool; none of these may be in use */
     while((dt = DefragTrackerDequeue(&defragtracker_spare_q))) {
         BUG_ON(SC_ATOMIC_GET(dt->use_cnt) > 0);
         DefragTrackerFree(dt);
     }
 
-    /* clear and free the hash */
+    /* free every tracker in the hash, then the hash */
     if (defragtracker_hash != NULL) {
         for (uint32_t u = 0; u < defrag_config.hash_size; u++) {
             dt = defragtracker_hash[u].head;
@@ -317,15 +345,12 @@ void DefragHashShutdown(void)
     DefragTrackerStackDestroy(&defragtracker_spare_q);
 }
 
-/** \brief compare two raw ipv6 addrs
- *
- *  \note we don't care about the real ipv6 ip's, this is just
- *        to consistently fill the DefragHashKey6 struct, without all
- *        the SCNtohl calls.
+/** \brief Order two raw IPv6 addresses.
  *
- *  \warning do not use elsewhere unless you know what you're doing.
- *           detect-engine-address-ipv6.c's AddressIPv6GtU32 is likely
- *           what you are looking for.
+ *  \note Only used to put the addresses in a fixed order in
+ *        DefragHashKey6, so both directions hash the same. Works on raw
+ *        (network order) words, so it is not a numeric compare.
+ *  \warning Do not reuse elsewhere: not a real comparison.
  */
 static inline int DefragHashRawAddressIPv6GtU32(const uint32_t *a, const uint32_t *b)
 {
@@ -339,6 +364,8 @@ static inline int DefragHashRawAddressIPv6GtU32(const uint32_t *a, const uint32_
     return 0;
 }
 
+/* Hash keys, overlaid with u32[] so hashword() can read them as words.
+ * pad must be zeroed. */
 typedef struct DefragHashKey4_ {
     union {
         struct {
@@ -363,14 +390,12 @@ typedef struct DefragHashKey6_ {
     };
 } DefragHashKey6;
 
-/* calculate the hash key for this packet
- *
- * we're using:
- *  hash_rand -- set at init time
- *  source address
- *  destination address
- *  id
- *  vlan_id
+/* Bucket index for this packet, hashed from:
+ *  - hash_rand (random seed set at init)
+ *  - source and destination addresses, sorted so both directions match
+ *  - fragment id (IPv4 IP ID or IPv6 fragment header id)
+ *  - vlan ids
+ * Non-IP packets go to bucket 0.
  */
 static inline uint32_t DefragHashGetKey(Packet *p)
 {
@@ -425,8 +450,9 @@ static inline uint32_t DefragHashGetKey(Packet *p)
     return key;
 }
 
-/* Since two or more trackers can have the same hash key, we need to compare
- * the tracker with the current tracker key. */
+/* Several trackers can share a bucket, so do a full match of tracker d1
+ * against packet d2: addresses (either direction), protocol, fragment
+ * id and vlan ids. */
 #define CMP_DEFRAGTRACKER(d1, d2, id)                                                              \
     (((CMP_ADDR(&(d1)->src_addr, &(d2)->src) && CMP_ADDR(&(d1)->dst_addr, &(d2)->dst)) ||          \
              (CMP_ADDR(&(d1)->src_addr, &(d2)->dst) && CMP_ADDR(&(d1)->dst_addr, &(d2)->src))) &&  \
@@ -434,6 +460,7 @@ static inline uint32_t DefragHashGetKey(Packet *p)
             (d1)->vlan_id[0] == (d2)->vlan_id[0] && (d1)->vlan_id[1] == (d2)->vlan_id[1] &&        \
             (d1)->vlan_id[2] == (d2)->vlan_id[2])
 
+/** \retval 1 tracker \p t belongs to packet \p p, 0 otherwise */
 static inline int DefragTrackerCompare(DefragTracker *t, Packet *p)
 {
     uint32_t id;
@@ -451,6 +478,7 @@ static inline int DefragTrackerCompare(DefragTracker *t, Packet *p)
     return CMP_DEFRAGTRACKER(t, p, id);
 }
 
+/** \brief Count a memcap exception policy hit, if that counter exists. */
 static void DefragExceptionPolicyStatsIncr(
         ThreadVars *tv, DecodeThreadVars *dtv, enum ExceptionPolicy policy)
 {
@@ -461,12 +489,13 @@ static void DefragExceptionPolicyStatsIncr(
 }
 
 /**
- *  \brief Get a new defrag tracker
+ *  \brief Get an empty tracker.
  *
- *  Get a new defrag tracker. We're checking memcap first and will try to make room
- *  if the memcap is reached.
+ *  Tries the spare pool, then a new allocation. At memcap, evicts an
+ *  idle tracker from the hash instead. If all fail, applies the memcap
+ *  exception policy to \p p. Caller holds the bucket lock.
  *
- *  \retval dt *LOCKED* tracker on success, NULL on error.
+ *  \retval dt *LOCKED* tracker on success, NULL on failure
  */
 static DefragTracker *DefragTrackerGetNew(ThreadVars *tv, DecodeThreadVars *dtv, Packet *p)
 {
@@ -481,10 +510,10 @@ static DefragTracker *DefragTrackerGetNew(ThreadVars *tv, DecodeThreadVars *dtv,
 
     DefragTracker *dt = NULL;
 
-    /* get a tracker from the spare queue */
+    /* first choice: a tracker from the spare pool */
     dt = DefragTrackerDequeue(&defragtracker_spare_q);
     if (dt == NULL) {
-        /* If we reached the max memcap, we get a used tracker */
+        /* at memcap: evict an idle tracker from the hash */
         if (!(DEFRAG_CHECK_MEMCAP(sizeof(DefragTracker)))) {
             dt = DefragTrackerGetUsedDefragTracker(tv, dtv);
             if (dt == NULL) {
@@ -493,9 +522,9 @@ static DefragTracker *DefragTrackerGetNew(ThreadVars *tv, DecodeThreadVars *dtv,
                 return NULL;
             }
 
-            /* freed a tracker, but it's unlocked */
+            /* evicted tracker is cleared and *unlocked* */
         } else {
-            /* now see if we can alloc a new tracker */
+            /* below memcap: allocate a new one */
             dt = DefragTrackerAlloc();
             if (dt == NULL) {
                 ExceptionPolicyApply(p, defrag_config.memcap_policy, PKT_DROP_REASON_DEFRAG_MEMCAP);
@@ -503,12 +532,12 @@ static DefragTracker *DefragTrackerGetNew(ThreadVars *tv, DecodeThreadVars *dtv,
                 return NULL;
             }
 
-            /* tracker is initialized but *unlocked* */
+            /* new tracker is zeroed and *unlocked* */
         }
     } else {
-        /* tracker has been recycled before it went into the spare queue */
+        /* spare trackers were cleared before entering the pool */
 
-        /* tracker is initialized (recycled) but *unlocked* */
+        /* so it is ready to use, and *unlocked* */
     }
 
     (void) SC_ATOMIC_ADD(defragtracker_counter, 1);
@@ -516,25 +545,27 @@ static DefragTracker *DefragTrackerGetNew(ThreadVars *tv, DecodeThreadVars *dtv,
     return dt;
 }
 
-/* DefragGetTrackerFromHash
+/** \brief Find the tracker for this packet, or create one.
  *
- * Hash retrieval function for trackers. Looks up the hash bucket containing the
- * tracker pointer. Then compares the packet with the found tracker to see if it is
- * the tracker we need. If it isn't, walk the list until the right tracker is found.
+ * Hashes the packet to a bucket and walks its chain for a match.
+ * Timed-out trackers met on the way are unlinked and moved to
+ * the spare pool. With no match, a new tracker is added at the
+ * head of the bucket.
  *
- * returns a *LOCKED* tracker or NULL
+ * \retval a *LOCKED* tracker with its use count raised, or NULL if no
+ * tracker could be had (memcap).
  */
 DefragTracker *DefragGetTrackerFromHash(ThreadVars *tv, DecodeThreadVars *dtv, Packet *p)
 {
     DefragTracker *dt = NULL;
 
-    /* get the key to our bucket */
+    /* get the bucket for this packet */
     uint32_t key = DefragHashGetKey(p);
-    /* get our hash bucket and lock it */
+    /* and lock it */
     DefragTrackerHashRow *hb = &defragtracker_hash[key];
     DRLOCK_LOCK(hb);
 
-    /* see if the bucket already has a tracker */
+    /* empty bucket: create the first tracker */
     if (hb->head == NULL) {
         dt = DefragTrackerGetNew(tv, dtv, p);
         if (dt == NULL) {
@@ -545,14 +576,14 @@ DefragTracker *DefragGetTrackerFromHash(ThreadVars *tv, DecodeThreadVars *dtv, P
         /* tracker is locked */
         hb->head = dt;
 
-        /* got one, now lock, initialize and return */
+        /* initialize and return it */
         DefragTrackerInit(dt,p);
 
         DRLOCK_UNLOCK(hb);
         return dt;
     }
 
-    /* ok, we have a tracker in the bucket. Let's find out if it is our tracker */
+    /* bucket has trackers: look for ours */
     DefragTracker *prev_dt = NULL;
     dt = hb->head;
 
@@ -575,19 +606,20 @@ DefragTracker *DefragGetTrackerFromHash(ThreadVars *tv, DecodeThreadVars *dtv, P
             StatsCounterIncr(&tv->stats, dtv->counter_defrag_tracker_timeout);
             goto tracker_removed;
         } else if (!dt->remove && DefragTrackerCompare(dt, p)) {
-            /* found our tracker, keep locked & return */
+            /* found it: return it still locked */
             (void)DefragTrackerIncrUsecnt(dt);
             DRLOCK_UNLOCK(hb);
             return dt;
         }
         SCMutexUnlock(&dt->lock);
-        /* unless we removed 'dt', prev_dt needs to point to
-         * current 'dt' when adding a new tracker below. */
+        /* prev_dt advances only when dt stays in the chain, so it is
+         * correct for unlinking on the next iteration. */
         prev_dt = dt;
         next_dt = dt->hnext;
 
     tracker_removed:
         if (next_dt == NULL) {
+            /* end of chain without a match: create a new tracker */
             dt = DefragTrackerGetNew(tv, dtv, p);
             if (dt == NULL) {
                 DRLOCK_UNLOCK(hb);
@@ -598,7 +630,7 @@ DefragTracker *DefragGetTrackerFromHash(ThreadVars *tv, DecodeThreadVars *dtv, P
 
             /* tracker is locked */
 
-            /* initialize and return */
+            /* initialize and return it */
             DefragTrackerInit(dt, p);
 
             DRLOCK_UNLOCK(hb);
@@ -613,34 +645,33 @@ DefragTracker *DefragGetTrackerFromHash(ThreadVars *tv, DecodeThreadVars *dtv, P
     return NULL;
 }
 
-/** \brief look up a tracker in the hash
- *
- *  \param a address to look up
+/** \brief Look up the tracker for a packet. Never creates one.
  *
- *  \retval h *LOCKED* tracker or NULL
+ *  \param p packet whose datagram to look up
+ *  \retval dt *LOCKED* tracker with use count raised, or NULL
  */
 DefragTracker *DefragLookupTrackerFromHash (Packet *p)
 {
     DefragTracker *dt = NULL;
 
-    /* get the key to our bucket */
+    /* get the bucket for this packet */
     uint32_t key = DefragHashGetKey(p);
-    /* get our hash bucket and lock it */
+    /* and lock it */
     DefragTrackerHashRow *hb = &defragtracker_hash[key];
     DRLOCK_LOCK(hb);
 
-    /* see if the bucket already has a tracker */
+    /* empty bucket: nothing to find */
     if (hb->head == NULL) {
         DRLOCK_UNLOCK(hb);
         return dt;
     }
 
-    /* ok, we have a tracker in the bucket. Let's find out if it is our tracker */
+    /* bucket has trackers: look for ours */
     dt = hb->head;
 
     do {
         if (!dt->remove && DefragTrackerCompare(dt, p)) {
-            /* found our tracker, lock & return */
+            /* found it: lock and return */
             SCMutexLock(&dt->lock);
             (void)DefragTrackerIncrUsecnt(dt);
             DRLOCK_UNLOCK(hb);
@@ -660,15 +691,16 @@ DefragTracker *DefragLookupTrackerFromHash (Packet *p)
 }
 
 /** \internal
- *  \brief Get a tracker from the hash directly.
+ *  \brief Evict a tracker from the hash for reuse.
  *
- *  Called in conditions where the spare queue is empty and memcap is reached.
+ *  Used when the spare pool is empty and the memcap is reached.
  *
- *  Walks the hash until a tracker can be freed. "defragtracker_prune_idx" atomic int makes
- *  sure we don't start at the top each time since that would clear the top of
- *  the hash leading to longer and longer search times under high pressure (observed).
+ *  Scans buckets for an idle tracker at a bucket head, skipping busy
+ *  locks. The scan resumes where the last one stopped
+ *  (defragtracker_prune_idx): always starting at bucket 0 would drain
+ *  the start of the hash and make each search longer under load (observed).
  *
- *  \retval dt tracker or NULL
+ *  \retval dt cleared, unlinked, *unlocked* tracker, or NULL if none
  */
 static DefragTracker *DefragTrackerGetUsedDefragTracker(ThreadVars *tv, const DecodeThreadVars *dtv)
 {
@@ -695,18 +727,19 @@ static DefragTracker *DefragTrackerGetUsedDefragTracker(ThreadVars *tv, const De
             continue;
         }
 
-        /** never prune a tracker that is used by a packets
-         *  we are currently processing in one of the threads */
+        /** never evict a tracker held by a packet that some thread is
+         *  still processing */
         if (SC_ATOMIC_GET(dt->use_cnt) > 0) {
             DRLOCK_UNLOCK(hb);
             SCMutexUnlock(&dt->lock);
             continue;
         }
 
-        /* only count "forced" reuse */
+        /* "hard" reuse: the tracker was still live. "Soft": it was
+         * already marked for removal. */
         bool incr_reuse_cnt = !dt->remove;
 
-        /* remove from the hash */
+        /* unlink from the bucket */
         hb->head = dt->hnext;
 
         dt->hnext = NULL;

src/detect-base64-data.c

@@ -15,6 +15,19 @@
  * 02110-1301, USA.
  */
 
+/**
+ * \file
+ *
+ * `base64_data` keyword. Takes no options. It must follow a
+ * `base64_decode`, and switches the rule's active list so that the
+ * content matches after it inspect the decoded data:
+ *
+ *   base64_decode; base64_data; content:"foo";
+ *
+ * The keyword itself matches nothing; it only changes where later
+ * keywords are added.
+ */
+
 #include "suricata-common.h"
 #include "detect.h"
 #include "detect-engine.h"
@@ -44,12 +57,18 @@ void DetectBase64DataRegister(void)
     sigmatch_table[DETECT_BASE64_DATA].flags |= SIGMATCH_NOOPT;
 }
 
+/**
+ * \brief Make the base64 decoded buffer as the active list for the
+ *        keywords that follow. No sigmatch is added.
+ *
+ * \retval 0 ok, -1 no `base64_decode` earlier in the rule
+ */
 static int DetectBase64DataSetup(DetectEngineCtx *de_ctx, Signature *s,
     const char *str)
 {
     SigMatch *pm = NULL;
 
-    /* Check for a preceding base64_decode. */
+    /* Require a base64_decode earlier in the rule, in any list. */
     pm = SCDetectGetLastSMFromLists(s, DETECT_BASE64_DECODE, -1);
     if (pm == NULL) {
         SCLogError("\"base64_data\" keyword seen without preceding base64_decode.");
@@ -62,8 +81,13 @@ static int DetectBase64DataSetup(DetectEngineCtx *de_ctx, Signature *s,
 
 #ifdef UNITTESTS
 
+/* set at test registration, not used by the current tests */
 static int g_file_data_buffer_id = 0;
 
+/**
+ * \test base64_decode stays in the payload list, and the content after
+ *       base64_data goes to the base64 data list.
+ */
 static int DetectBase64DataSetupTest01(void)
 {
     DetectEngineCtx *de_ctx = DetectEngineCtxInit();
@@ -85,8 +109,8 @@ static int DetectBase64DataSetupTest01(void)
 }
 
 /**
- * \test Test that the list can be changed to post-detection lists
- *     after the base64 keyword.
+ * \test A post-match keyword (tag) is still accepted after base64_data,
+ *       with base64_decode inside a file_data buffer.
  */
 static int DetectBase64DataSetupTest04(void)
 {

src/detect-base64-data.h

@@ -15,9 +15,17 @@
  * 02110-1301, USA.
  */
 
+/**
+ * \file
+ *
+ * `base64_data` rule keyword: following content matches inspect the
+ * buffer decoded by a preceding `base64_decode`.
+ */
+
 #ifndef SURICATA_DETECT_BASE64_DATA_H
 #define SURICATA_DETECT_BASE64_DATA_H
 
+/** \brief Register the `base64_data` keyword in sigmatch_table. */
 void DetectBase64DataRegister(void);
 
 #endif /* SURICATA_DETECT_BASE64_DATA_H */

src/detect-base64-decode.c

@@ -15,6 +15,21 @@
  * 02110-1301, USA.
  */
 
+/**
+ * \file
+ *
+ * `base64_decode` keyword. Decodes part of the current buffer into a
+ * per-thread buffer that `base64_data` then inspects:
+ *
+ *   base64_decode[: [bytes <n>][, offset <n>][, relative]];
+ *
+ *   bytes     max input bytes to decode (default and max: 65535)
+ *   offset    skip this many bytes first
+ *   relative  start after the previous match instead of the buffer start
+ *
+ * Options are all optional but must appear in this order.
+ */
+
 #include "suricata-common.h"
 #include "detect.h"
 #include "detect-parse.h"
@@ -24,15 +39,19 @@
 #include "detect-engine-build.h"
 #include "rust.h"
 
-/* Arbitrary maximum buffer size for decoded base64 data. */
+/* Arbitrary maximum buffer size for decoded base64 data. Also the
+ * default for `bytes`. */
 #define BASE64_DECODE_MAX 65535
 
+/** parsed keyword options */
 typedef struct DetectBase64Decode_ {
-    uint16_t bytes;
-    uint32_t offset;
-    uint8_t relative;
+    uint16_t bytes;   /**< max input bytes to decode */
+    uint32_t offset;  /**< bytes to skip before decoding */
+    uint8_t relative; /**< 1: offset counts from the previous match */
 } DetectBase64Decode;
 
+/* "bytes N", "offset N" and a trailing word, in that order, each
+ * optional. The trailing word must be "relative" (checked when parsing). */
 static const char decode_pattern[] = "\\s*(bytes\\s+(\\d+),?)?"
     "\\s*(offset\\s+(\\d+),?)?"
     "\\s*(\\w+)?";
@@ -63,6 +82,16 @@ void DetectBase64DecodeRegister(void)
     DetectSetupParseRegexes(decode_pattern, &decode_pcre);
 }
 
+/**
+ * \brief Decode the selected part of \p payload (RFC 4648) into
+ *        det_ctx->base64_decoded.
+ *
+ * Skips to the previous match if `relative`, then skips `offset`, then
+ * decodes at most `bytes` bytes.
+ *
+ * \retval 1 det_ctx->base64_decoded_len > 0, 0 otherwise (including
+ *         offset past the end of the buffer)
+ */
 int DetectBase64DecodeDoMatch(DetectEngineThreadCtx *det_ctx, const Signature *s,
     const SigMatchData *smd, const uint8_t *payload, uint32_t payload_len)
 {
@@ -110,6 +139,10 @@ int DetectBase64DecodeDoMatch(DetectEngineThreadCtx *det_ctx, const Signature *s
     return det_ctx->base64_decoded_len > 0;
 }
 
+/**
+ * \brief Parse the keyword options. Unset options are left at 0.
+ * \retval 1 ok, 0 parse error (unknown word, bad number, empty string)
+ */
 static int DetectBase64DecodeParse(
         const char *str, uint16_t *bytes, uint32_t *offset, uint8_t *relative)
 {
@@ -182,6 +215,17 @@ error:
     return retval;
 }
 
+/**
+ * \brief Parse options and add the keyword to the rule.
+ *
+ * The keyword goes to the active list (e.g. after file_data), else to
+ * the list of the last payload keyword (content, pcre, byte_*,
+ * isdataat), else to the packet payload list. Also records the largest
+ * `bytes` in de_ctx->base64_decode_max_len, which sizes the per-thread
+ * decode buffer.
+ *
+ * \retval 0 ok, -1 error
+ */
 static int DetectBase64DecodeSetup(DetectEngineCtx *de_ctx, Signature *s,
     const char *str)
 {
@@ -227,6 +271,7 @@ static int DetectBase64DecodeSetup(DetectEngineCtx *de_ctx, Signature *s,
         goto error;
     }
 
+    /* bytes 0 or unset means "as much as allowed" */
     if (!data->bytes) {
         data->bytes = BASE64_DECODE_MAX;
     }
@@ -256,8 +301,11 @@ static void DetectBase64DecodeFree(DetectEngineCtx *de_ctx, void *ptr)
 #include "flow-util.h"
 #include "stream-tcp.h"
 
+/* set at test registration, not used by the current tests */
 static int g_http_header_buffer_id = 0;
 
+/** \test Option parsing: valid combinations, then misspellings and an
+ *        empty string, which must fail. */
 static int DetectBase64TestDecodeParse(void)
 {
     int retval = 0;
@@ -328,7 +376,7 @@ static int DetectBase64TestDecodeParse(void)
         goto end;
     }
 
-    /* Misspelled empty string. */
+    /* Empty string. */
     if (DetectBase64DecodeParse("", &bytes, &offset, &relative)) {
         goto end;
     }
@@ -339,7 +387,8 @@ end:
 }
 
 /**
- * Test keyword setup on basic content.
+ * \test Keyword setup with no options, followed by a content: the
+ *       keyword lands in the payload list.
  */
 static int DetectBase64DecodeTestSetup(void)
 {
@@ -356,6 +405,7 @@ static int DetectBase64DecodeTestSetup(void)
     PASS;
 }
 
+/** \test Decoding a fully base64 payload produces output. */
 static int DetectBase64DecodeTestDecode(void)
 {
     ThreadVars tv;
@@ -411,6 +461,7 @@ end:
     return retval;
 }
 
+/** \test `offset 8` skips the 8-byte prefix and decodes "Hello World". */
 static int DetectBase64DecodeTestDecodeWithOffset(void)
 {
     ThreadVars tv;
@@ -473,6 +524,7 @@ end:
     return retval;
 }
 
+/** \test An offset past the end of the payload decodes nothing. */
 static int DetectBase64DecodeTestDecodeLargeOffset(void)
 {
     ThreadVars tv;
@@ -493,7 +545,7 @@ static int DetectBase64DecodeTestDecodeLargeOffset(void)
         goto end;
     }
 
-    /* Offset is out of range. */
+    /* Offset 32 is past the end of the 16-byte payload. */
     de_ctx->sig_list = SigInit(de_ctx,
         "alert tcp any any -> any any (msg:\"base64 test\"; "
         "base64_decode: bytes 16, offset 32; "
@@ -529,6 +581,8 @@ end:
     return retval;
 }
 
+/** \test `relative` starts decoding right after the preceding content
+ *        match and decodes "Hello World". */
 static int DetectBase64DecodeTestDecodeRelative(void)
 {
     ThreadVars tv;

src/detect-base64-decode.h

@@ -15,10 +15,22 @@
  * 02110-1301, USA.
  */
 
+/**
+ * \file
+ *
+ * `base64_decode` rule keyword: base64-decodes part of the current
+ * buffer so `base64_data` can match on the result.
+ */
+
 #ifndef SURICATA_DETECT_BASE64_DECODE_H
 #define SURICATA_DETECT_BASE64_DECODE_H
 
+/** \brief Register the `base64_decode` keyword in sigmatch_table. */
 void DetectBase64DecodeRegister(void);
+
+/** \brief Decode part of \p payload into det_ctx->base64_decoded.
+ *  Called by the content inspection engine.
+ *  \retval 1 something was decoded, 0 otherwise */
 int DetectBase64DecodeDoMatch(DetectEngineThreadCtx *, const Signature *,
     const SigMatchData *, const uint8_t *, uint32_t);
 

src/detect-dnp3.c

@@ -15,6 +15,19 @@
  * 02110-1301, USA.
  */
 
+/**
+ * \file
+ *
+ * DNP3 rule keywords:
+ *
+ *   dnp3.data  sticky buffer: the reassembled application data
+ *   dnp3_func  application function code (uint8, names allowed)
+ *   dnp3_ind   internal indication flags, responses only (uint16)
+ *   dnp3_obj   object group and variation: dnp3_obj:<group>,<variation>
+ *
+ * Each keyword also has a dotted or underscored alias.
+ */
+
 #include "suricata-common.h"
 
 #include "stream.h"
@@ -32,15 +45,16 @@
 #include "app-layer-dnp3.h"
 #include "util-byte.h"
 
+/* list ids: "dnp3" (func and obj), "dnp3_data", "dnp3_ind" */
 static int g_dnp3_match_buffer_id = 0;
 static int g_dnp3_data_buffer_id = 0;
 static int g_dnp3_ind_buffer_id = 0;
 
 /**
- * The detection struct.
+ * dnp3_obj match data.
  */
 typedef struct DetectDNP3_ {
-    /* Object info for object detection. */
+    /* Object to look for in the transaction. */
     uint8_t obj_group;
     uint8_t obj_variation;
 } DetectDNP3;
@@ -50,6 +64,9 @@ static void DetectDNP3FuncRegisterTests(void);
 static void DetectDNP3ObjRegisterTests(void);
 #endif
 
+/** \brief dnp3.data buffer getter: the tx's reassembled application
+ *  data, for requests to server and responses to client only.
+ *  \return the buffer, or NULL if wrong direction or no data */
 static InspectionBuffer *GetDNP3Data(DetectEngineThreadCtx *det_ctx,
         const DetectEngineTransforms *transforms,
         Flow *_f, const uint8_t flow_flags,
@@ -82,6 +99,8 @@ static void DetectDNP3FuncFree(DetectEngineCtx *de_ctx, void *ptr)
     SCDetectU8Free(ptr);
 }
 
+/** \brief dnp3_func setup: parse a uint8 expression or function name.
+ *  \retval 0 ok, -1 error */
 static int DetectDNP3FuncSetup(DetectEngineCtx *de_ctx, Signature *s, const char *str)
 {
     SCEnter();
@@ -113,6 +132,8 @@ static void DetectDNP3IndFree(DetectEngineCtx *de_ctx, void *ptr)
     SCDetectU16Free(ptr);
 }
 
+/** \brief dnp3_ind setup: parse a uint16 expression or flag names.
+ *  \retval 0 ok, -1 error */
 static int DetectDNP3IndSetup(DetectEngineCtx *de_ctx, Signature *s, const char *str)
 {
     SCEnter();
@@ -140,13 +161,15 @@ error:
 }
 
 /**
- * \brief Parse the value of string of the dnp3_obj keyword.
+ * \brief Parse the dnp3_obj value "<group>,<variation>".
+ *
+ * Each number is 0-255; hex (0x..) and octal (0..) are accepted.
  *
  * \param str the input string
- * \param gout pointer to variable to store the parsed group integer
- * \param vout pointer to variable to store the parsed variation integer
+ * \param group where to store the parsed group
+ * \param var where to store the parsed variation
  *
- * \retval 1 if parsing successful otherwise 0.
+ * \retval 1 on success, 0 on failure
  */
 static int DetectDNP3ObjParse(const char *str, uint8_t *group, uint8_t *var)
 {
@@ -172,6 +195,8 @@ static int DetectDNP3ObjParse(const char *str, uint8_t *group, uint8_t *var)
     return 1;
 }
 
+/** \brief dnp3_obj setup.
+ *  \retval 1 ok, 0 error. Note: not the usual 0 / -1 convention. */
 static int DetectDNP3ObjSetup(DetectEngineCtx *de_ctx, Signature *s, const char *str)
 {
     SCEnter();
@@ -206,6 +231,7 @@ fail:
     SCReturnInt(0);
 }
 
+/** \brief Free dnp3_obj match data. */
 static void DetectDNP3Free(DetectEngineCtx *de_ctx, void *ptr)
 {
     SCEnter();
@@ -215,6 +241,8 @@ static void DetectDNP3Free(DetectEngineCtx *de_ctx, void *ptr)
     SCReturn;
 }
 
+/** \brief Match the function code of a request (to server) or a
+ *  response (to client). */
 static int DetectDNP3FuncMatch(DetectEngineThreadCtx *det_ctx,
     Flow *f, uint8_t flags, void *state, void *txv, const Signature *s,
     const SigMatchCtx *ctx)
@@ -231,6 +259,8 @@ static int DetectDNP3FuncMatch(DetectEngineThreadCtx *det_ctx,
     return 0;
 }
 
+/** \brief Match if the tx contains an object with the given group and
+ *  variation (same direction rule as dnp3_func). */
 static int DetectDNP3ObjMatch(DetectEngineThreadCtx *det_ctx,
     Flow *f, uint8_t flags, void *state, void *txv, const Signature *s,
     const SigMatchCtx *ctx)
@@ -258,6 +288,8 @@ static int DetectDNP3ObjMatch(DetectEngineThreadCtx *det_ctx,
     return 0;
 }
 
+/** \brief Match the IIN flags as one uint16 (IIN1 high byte, IIN2 low).
+ *  No direction check: the dnp3_ind list is only inspected to client. */
 static int DetectDNP3IndMatch(DetectEngineThreadCtx *det_ctx,
     Flow *f, uint8_t flags, void *state, void *txv, const Signature *s,
     const SigMatchCtx *ctx)
@@ -329,6 +361,8 @@ static void DetectDNP3ObjRegister(void)
     SCReturn;
 }
 
+/** \brief dnp3.data setup: make dnp3_data the active buffer.
+ *  \retval 0 ok, -1 error */
 static int DetectDNP3DataSetup(DetectEngineCtx *de_ctx, Signature *s, const char *str)
 {
     SCEnter();
@@ -341,6 +375,7 @@ static int DetectDNP3DataSetup(DetectEngineCtx *de_ctx, Signature *s, const char
     SCReturnInt(0);
 }
 
+/* dnp3.data: inspection and prefilter (mpm) engines, both directions */
 static void DetectDNP3DataRegister(void)
 {
     SCEnter();
@@ -367,6 +402,7 @@ static void DetectDNP3DataRegister(void)
     SCReturn;
 }
 
+/** \brief Register all DNP3 keywords and their inspection lists. */
 void DetectDNP3Register(void)
 {
     DetectDNP3DataRegister();
@@ -375,7 +411,7 @@ void DetectDNP3Register(void)
     DetectDNP3IndRegister();
     DetectDNP3ObjRegister();
 
-    /* Register the list of func, ind and obj. */
+    /* "dnp3" list, shared by func and obj, inspected in both directions. */
     DetectAppLayerInspectEngineRegister(
             "dnp3", ALPROTO_DNP3, SIG_FLAG_TOSERVER, 0, DetectEngineInspectGenericList, NULL);
     DetectAppLayerInspectEngineRegister(
@@ -383,6 +419,7 @@ void DetectDNP3Register(void)
 
     g_dnp3_match_buffer_id = DetectBufferTypeRegister("dnp3");
 
+    /* "dnp3_ind" list: IIN flags exist only in responses, so to client only. */
     DetectAppLayerInspectEngineRegister(
             "dnp3_ind", ALPROTO_DNP3, SIG_FLAG_TOCLIENT, 0, DetectEngineInspectGenericList, NULL);
     g_dnp3_ind_buffer_id = DetectBufferTypeRegister("dnp3_ind");
@@ -395,6 +432,7 @@ void DetectDNP3Register(void)
 #include "flow-util.h"
 #include "stream-tcp.h"
 
+/** \test dnp3_func:2 lands in the dnp3 list with value 2. */
 static int DetectDNP3FuncTest01(void)
 {
     DetectEngineCtx *de_ctx = DetectEngineCtxInit();
@@ -416,6 +454,7 @@ static int DetectDNP3FuncTest01(void)
     PASS;
 }
 
+/** \test dnp3_obj:99,99 lands in the dnp3 list with group and variation 99. */
 static int DetectDNP3ObjSetupTest(void)
 {
     DetectEngineCtx *de_ctx = DetectEngineCtxInit();
@@ -438,6 +477,8 @@ static int DetectDNP3ObjSetupTest(void)
     PASS;
 }
 
+/** \test dnp3_obj parsing: 0-255 accepted; negatives, overflow and
+ *        non-numbers rejected. */
 static int DetectDNP3ObjParseTest(void)
 {
     uint8_t group, var;