From: Florian Obser <florian@openbsd.org>
Subject: Re: dhcpleased(8): do not peek inside of struct imsg
To: Theo Buehler <tb@theobuehler.org>
Cc: tech <tech@openbsd.org>
Date: Sun, 25 Aug 2024 15:32:54 +0200

On 2024-08-25 10:57 +02, Theo Buehler <tb@theobuehler.org> wrote:
>> @@ -491,44 +493,48 @@ engine_dispatch_main(int fd, short event, void *bula)
>>  			break;
>>  		case IMSG_RECONF_VC_ID:
>>  			if (iface_conf == NULL)
>> -				fatal("IMSG_RECONF_VC_ID without "
>> +				fatalx("IMSG_RECONF_VC_ID without "
>>  				    "IMSG_RECONF_IFACE");
>
> should these messages include __func__ for consistency with most other
> fatals in here?
>
> I was wondering if it makes sense to make this slightly stricter and add:
>
> 			if (iface_conf->vc_id != NULL)
> 				fatalx("multiple IMSG_RECONF_VC_ID "
> 				    "for the same interface");
>
> otherwise the following malloc() looks like a potential leak.
>
> If you do this, of course C_ID and H_NAME should get the same treatment,
> also in the frontend. All this would better be in a separate changeset.
>

All of that makes sense.

OK?

(It might be a bit silly to go through i2s, but that results in less
awkward line breaks, I think.)

diff --git engine.c engine.c
index ec54aa979f3..e8a8cbd0086 100644
--- engine.c
+++ engine.c
@@ -493,8 +493,11 @@ engine_dispatch_main(int fd, short event, void *bula)
 			break;
 		case IMSG_RECONF_VC_ID:
 			if (iface_conf == NULL)
-				fatalx("IMSG_RECONF_VC_ID without "
-				    "IMSG_RECONF_IFACE");
+				fatalx("%s: %s without IMSG_RECONF_IFACE",
+				    __func__, i2s(type));
+			if (iface_conf->vc_id != NULL)
+				fatalx("%s: multiple %s for the same interface",
+				    __func__, i2s(type));
 			if ((iface_conf->vc_id_len = imsg_get_len(&imsg))
 			    > 255 + 2 || iface_conf->vc_id_len == 0)
 				fatalx("%s: invalid %s", __func__, i2s(type));
@@ -507,8 +510,11 @@ engine_dispatch_main(int fd, short event, void *bula)
 			break;
 		case IMSG_RECONF_C_ID:
 			if (iface_conf == NULL)
-				fatalx("IMSG_RECONF_C_ID without "
-				    "IMSG_RECONF_IFACE");
+				fatalx("%s: %s without IMSG_RECONF_IFACE",
+				    __func__, i2s(type));
+			if (iface_conf->c_id != NULL)
+				fatalx("%s: multiple %s for the same interface",
+				    __func__, i2s(type));
 			if ((iface_conf->c_id_len = imsg_get_len(&imsg))
 			    > 255 + 2 || iface_conf->c_id_len == 0)
 				fatalx("%s: invalid %s", __func__, i2s(type));
@@ -523,8 +529,11 @@ engine_dispatch_main(int fd, short event, void *bula)
 			size_t	len;
 
 			if (iface_conf == NULL)
-				fatalx("IMSG_RECONF_H_NAME without "
-				    "IMSG_RECONF_IFACE");
+				fatalx("%s: %s without IMSG_RECONF_IFACE",
+				    __func__, i2s(type));
+			if (iface_conf->h_name != NULL)
+				fatalx("%s: multiple %s for the same interface",
+				    __func__, i2s(type));
 			if ((len = imsg_get_len(&imsg)) > 256 || len == 0)
 				fatalx("%s: invalid %s", __func__, i2s(type));
 			if ((iface_conf->h_name = malloc(len)) == NULL)
diff --git frontend.c frontend.c
index d0d651dfb28..4fbcab21f69 100644
--- frontend.c
+++ frontend.c
@@ -369,8 +369,11 @@ frontend_dispatch_main(int fd, short event, void *bula)
 			break;
 		case IMSG_RECONF_VC_ID:
 			if (iface_conf == NULL)
-				fatalx("IMSG_RECONF_VC_ID without "
-				    "IMSG_RECONF_IFACE");
+				fatalx("%s: %s without IMSG_RECONF_IFACE",
+				    __func__, i2s(type));
+			if (iface_conf->vc_id != NULL)
+				fatalx("%s: multiple %s for the same interface",
+				    __func__, i2s(type));
 			if ((iface_conf->vc_id_len = imsg_get_len(&imsg))
 			    > 255 + 2 || iface_conf->vc_id_len == 0)
 				fatalx("%s: invalid %s", __func__, i2s(type));
@@ -383,8 +386,11 @@ frontend_dispatch_main(int fd, short event, void *bula)
 			break;
 		case IMSG_RECONF_C_ID:
 			if (iface_conf == NULL)
-				fatalx("IMSG_RECONF_C_ID without "
-				    "IMSG_RECONF_IFACE");
+				fatalx("%s: %s without IMSG_RECONF_IFACE",
+				    __func__, i2s(type));
+			if (iface_conf->c_id != NULL)
+				fatalx("%s: multiple %s for the same interface",
+				    __func__, i2s(type));
 			if ((iface_conf->c_id_len = imsg_get_len(&imsg))
 			    > 255 + 2 || iface_conf->c_id_len == 0)
 				fatalx("%s: invalid %s", __func__, i2s(type));
@@ -399,8 +405,11 @@ frontend_dispatch_main(int fd, short event, void *bula)
 			size_t	len;
 
 			if (iface_conf == NULL)
-				fatalx("IMSG_RECONF_H_NAME without "
-				    "IMSG_RECONF_IFACE");
+				fatalx("%s: %s without IMSG_RECONF_IFACE",
+				    __func__, i2s(type));
+			if (iface_conf->h_name != NULL)
+				fatalx("%s: multiple %s for the same interface",
+				    __func__, i2s(type));
 			if ((len = imsg_get_len(&imsg)) > 256 || len == 0)
 				fatalx("%s: invalid %s", __func__, i2s(type));
 			if ((iface_conf->h_name = malloc(len)) == NULL)


-- 
In my defence, I have been left unsupervised.