From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 935E1BE2A for ; Fri, 8 Apr 2022 10:41:12 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 8A465C909 for ; Fri, 8 Apr 2022 10:41:12 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 1AFE0C900 for ; Fri, 8 Apr 2022 10:41:09 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id DEB7845A1B for ; Fri, 8 Apr 2022 10:41:08 +0200 (CEST) Date: Fri, 08 Apr 2022 10:40:57 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20220317114415.3161589-1-d.csapak@proxmox.com> In-Reply-To: <20220317114415.3161589-1-d.csapak@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1649406844.4azt6nsvob.astroid@nora.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.176 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pve-devel] [PATCH cluster v3 1/2] add CFS_IPC_GET_GUEST_CONFIG_PROPERTIES method X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 08 Apr 2022 08:41:12 -0000 On March 17, 2022 12:44 pm, Dominik Csapak wrote: > for getting multiple properties from the in memory config of the > guests. I added a new CSF_IPC_ call to maintain backwards compatibility. >=20 > It basically behaves the same as > CFS_IPC_GET_GUEST_CONFIG_PROPERTY, but takes a list of properties > instead. >=20 > The old way of getting a single property is now also done by > the new function. >=20 > Signed-off-by: Dominik Csapak one more small nit, otherwise this LGTM > --- > changes since v2: > * add 'a-z' check while parsing the properties, this way the later min/ma= x > code works as intended >=20 > data/src/cfs-ipc-ops.h | 2 + > data/src/server.c | 62 +++++++++++++++ > data/src/status.c | 174 ++++++++++++++++++++++++++++------------- > data/src/status.h | 3 + > 4 files changed, 185 insertions(+), 56 deletions(-) >=20 > diff --git a/data/src/cfs-ipc-ops.h b/data/src/cfs-ipc-ops.h > index 003e233..249308d 100644 > --- a/data/src/cfs-ipc-ops.h > +++ b/data/src/cfs-ipc-ops.h > @@ -43,4 +43,6 @@ > =20 > #define CFS_IPC_VERIFY_TOKEN 12 > =20 > +#define CFS_IPC_GET_GUEST_CONFIG_PROPERTIES 13 > + > #endif > diff --git a/data/src/server.c b/data/src/server.c > index 549788a..b9394c8 100644 > --- a/data/src/server.c > +++ b/data/src/server.c > @@ -89,6 +89,13 @@ typedef struct { > char property[]; > } cfs_guest_config_propery_get_request_header_t; > =20 > +typedef struct { > + struct qb_ipc_request_header req_header; > + uint32_t vmid; > + uint8_t num_props; > + char props[]; /* list of \0 terminated properties */ > +} cfs_guest_config_properties_get_request_header_t; > + > typedef struct { > struct qb_ipc_request_header req_header; > char token[]; > @@ -348,6 +355,61 @@ static int32_t s1_msg_process_fn( > =20 > result =3D cfs_create_guest_conf_property_msg(outbuf, memdb, rh->prop= erty, rh->vmid); > } > + } else if (request_id =3D=3D CFS_IPC_GET_GUEST_CONFIG_PROPERTIES) { > + > + cfs_guest_config_properties_get_request_header_t *rh =3D > + (cfs_guest_config_properties_get_request_header_t *) data; > + > + int propslen =3D request_size - G_STRUCT_OFFSET(cfs_guest_config_prope= rties_get_request_header_t, props); > + > + result =3D 0; > + if (rh->vmid < 100 && rh->vmid !=3D 0) { > + cfs_debug("vmid out of range %u", rh->vmid); > + result =3D -EINVAL; sanity check for num_props being > 0 here would probably be good. with=20 the current code it's not problematic, but who knows how this evolves in=20 the future ;) (alternatively, we could also drop num_props entirely and just say the=20 msg must at most contain 255 properties, and always allocate 255 slots=20 below and abort on the 256th property? but I guess the safeguard against=20 bugs on the caller side doesn't hurt and it saves a bit of memory in the=20 common case which is probably with <10 properties even if we find a few=20 more use cases :)) > + } else if (rh->vmid >=3D 100 && !vmlist_vm_exists(rh->vmid)) { > + result =3D -ENOENT; > + } else if (propslen <=3D 1) { > + cfs_debug("propslen <=3D 1, %d", propslen); > + result =3D -EINVAL; > + } else { > + const char **properties =3D malloc(sizeof(char*) * rh->num_props); malloc(0), so we must never dereference the returned pointer in that=20 case. > + char *current =3D (rh->props); > + int remaining =3D propslen; > + for (uint8_t i =3D 0; i < rh->num_props; i++) { loop is skipped since 0 < 0 is false > + uint8_t proplen =3D strnlen(current, remaining); > + if (proplen =3D=3D 0) { > + cfs_debug("property length 0"); > + result =3D -EINVAL; > + break; > + } > + if (proplen =3D=3D remaining) { > + cfs_debug("property not \\0 terminated"); > + result =3D -EINVAL; > + break; > + } > + if (current[0] < 'a' || current[0] > 'z') { > + cfs_debug("property does not start with [a-z]"); > + result =3D -EINVAL; > + break; > + } > + properties[i] =3D current; > + current[proplen] =3D '\0'; // ensure property is 0 terminated > + remaining -=3D (proplen + 1); > + current +=3D proplen + 1; > + } > + > + if (remaining !=3D 0) { this must then be true, since we had data but didn't process it in any=20 way > + cfs_debug("leftover data after parsing %ul properties", rh->num_p= rops); > + result =3D -EINVAL; > + } > + > + if (result =3D=3D 0) { > + cfs_debug("cfs_get_guest_config_properties: basic valid checked, = do request"); > + result =3D cfs_create_guest_conf_properties_msg(outbuf, memdb, pr= operties, rh->num_props, rh->vmid); > + } > + > + free(properties); so we free it here without ever touching it and return an error. since=20 about the only thing we are allowed to do with the result of malloc(0)=20 is freeing it, right now this is okay :) > + } > } else if (request_id =3D=3D CFS_IPC_VERIFY_TOKEN) { > =20 > cfs_verify_token_request_header_t *rh =3D (cfs_verify_token_request_he= ader_t *) data; [..]