fix: dev: fix possible NULL dereference in cfg_map_findclause()

`cfg_map_findclause()` did not check whether a clause existed before dereferencing it, which could lead to a NULL dereference. Add the missing check to prevent this.

In practice, this was not triggering any known bug, since `cfg_map_findclause()` is only called in contexts where the clause is known to exist.

Closes #5997

Merge branch '5997-findclause' into 'main'

See merge request isc-projects/bind9!12052

lib/isccfg/include/isccfg/cfg.h

@@ -136,10 +136,10 @@ cfg_parser_currentfile(cfg_parser_t *pctx);
  */
 
 isc_result_t
-cfg_map_addclone(cfg_obj_t *map, const cfg_obj_t *obj,
-		 const cfg_clausedef_t *clause);
+cfg_map_add(cfg_obj_t *map, const cfg_obj_t *obj,
+	    const cfg_clausedef_t *clause);
 /*%<
- * Add a clone of 'obj' to the specified clause in mapbody 'mapobj'.
+ * Clone 'obj' and add its clone to the specified clause in mapbody 'mapobj'.
  * If the clause is tagged with CFG_CLAUSEFLAG_MULTI, the function expects
  * that 'obj' is a list and will clone each element and sequentially add them
  * (preserving the order), instead of adding a list as single element of
@@ -219,17 +219,6 @@ cfg_map_get(const cfg_obj_t *mapobj, const char *name, const cfg_obj_t **obj);
  * \li     #ISC_R_NOTFOUND                 - name not found in map
  */
 
-isc_result_t
-cfg_map_add(cfg_obj_t *mapobj, cfg_obj_t *obj, const char *clause);
-/*%<
- * Add the object 'obj' to the specified clause in mapbody 'mapobj'.
- * Used for adding new zones.
- *
- * Require:
- * \li     'obj' is a valid cfg_obj_t.
- * \li     'mapobj' is a valid cfg_obj_t of type map.
- */
-
 const cfg_obj_t *
 cfg_map_getname(const cfg_obj_t *mapobj);
 /*%<

lib/isccfg/namedconf.c

@@ -1176,8 +1176,8 @@ map_merge(const cfg_obj_t *config ISC_ATTR_UNUSED, cfg_obj_t *effectivemap,
 		if (effectiveres == ISC_R_NOTFOUND &&
 		    defaultres == ISC_R_SUCCESS)
 		{
-			INSIST(cfg_map_addclone(effectivemap, defaultobj,
-						clause) == ISC_R_SUCCESS);
+			INSIST(cfg_map_add(effectivemap, defaultobj, clause) ==
+			       ISC_R_SUCCESS);
 			continue;
 		}
 
@@ -1205,7 +1205,7 @@ cloneto(cfg_obj_t *options, const cfg_obj_t *obj, const char *clausename) {
 	const cfg_clausedef_t *clause = cfg_map_findclause(options->type,
 							   clausename);
 
-	result = cfg_map_addclone(options, obj, clause);
+	result = cfg_map_add(options, obj, clause);
 	INSIST(result == ISC_R_SUCCESS);
 }
 

lib/isccfg/parser.c

@@ -2948,10 +2948,14 @@ cfg_map_findclause(const cfg_type_t *map, const char *name) {
 	REQUIRE(name != NULL);
 
 	found = cfg_map_firstclause(map, &clauses, &idx);
-	while (name != NULL && strcasecmp(name, found->name)) {
+	while (found != NULL && name != NULL && strcasecmp(name, found->name)) {
 		found = cfg_map_nextclause(map, &clauses, &idx);
 	}
 
+	if (found == NULL) {
+		return found;
+	}
+
 	return ((cfg_clausedef_t *)clauses) + idx;
 }
 
@@ -4043,25 +4047,8 @@ map_define(cfg_obj_t *mapobj, cfg_obj_t *obj, const cfg_clausedef_t *clause) {
 }
 
 isc_result_t
-cfg_map_add(cfg_obj_t *mapobj, cfg_obj_t *obj, const char *clausename) {
-	const cfg_clausedef_t *clause;
-
-	REQUIRE(VALID_CFGOBJ(obj));
-	REQUIRE(VALID_CFGOBJ(mapobj));
-	REQUIRE(mapobj->type->rep == &cfg_rep_map);
-	REQUIRE(clausename != NULL);
-
-	clause = cfg_map_findclause(mapobj->type, clausename);
-	if (clause == NULL || clause->name == NULL) {
-		return ISC_R_FAILURE;
-	}
-
-	return map_define(mapobj, obj, clause);
-}
-
-isc_result_t
-cfg_map_addclone(cfg_obj_t *map, const cfg_obj_t *obj,
-		 const cfg_clausedef_t *clause) {
+cfg_map_add(cfg_obj_t *map, const cfg_obj_t *obj,
+	    const cfg_clausedef_t *clause) {
 	isc_result_t result = ISC_R_SUCCESS;
 	cfg_obj_t *clone = NULL;
 

tests/isccfg/parser_test.c

@@ -303,6 +303,18 @@ view \"_bind\" chaos {\n\
 	cfg_obj_detach(&clone);
 }
 
+static const cfg_clausedef_t *const empty_clausesets[] = { NULL };
+
+static cfg_type_t cfg_type_empty_map = {
+	"empty_map", NULL, NULL, NULL, &cfg_rep_map, &empty_clausesets,
+};
+
+ISC_RUN_TEST_IMPL(cfg_map_findclause_empty) {
+	const cfg_clausedef_t *result = cfg_map_findclause(&cfg_type_empty_map,
+							   "anything");
+	assert_null(result);
+}
+
 ISC_TEST_LIST_START
 
 ISC_TEST_ENTRY(addzoneconf)
@@ -310,6 +322,7 @@ ISC_TEST_ENTRY(parse_buffer)
 ISC_TEST_ENTRY(cfg_map_firstclause)
 ISC_TEST_ENTRY(cfg_map_nextclause)
 ISC_TEST_ENTRY(cfg_clone_copy)
+ISC_TEST_ENTRY(cfg_map_findclause_empty)
 
 ISC_TEST_LIST_END