Re: [pve-devel] [PATCH cluster v3 1/2] add CFS_IPC_GET_GUEST_CONFIG_PROPERTIES method

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

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.
> 
> It basically behaves the same as
> CFS_IPC_GET_GUEST_CONFIG_PROPERTY, but takes a list of properties
> instead.
> 
> The old way of getting a single property is now also done by
> the new function.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>

one more small nit, otherwise this LGTM

> ---
> changes since v2:
> * add 'a-z' check while parsing the properties, this way the later min/max
>   code works as intended
> 
>  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(-)
> 
> 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 @@
>  
>  #define CFS_IPC_VERIFY_TOKEN 12
>  
> +#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;
>  
> +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(
>  
>  			result = cfs_create_guest_conf_property_msg(outbuf, memdb, rh->property, rh->vmid);
>  		}
> +	} else if (request_id == CFS_IPC_GET_GUEST_CONFIG_PROPERTIES) {
> +
> +		cfs_guest_config_properties_get_request_header_t *rh =
> +			(cfs_guest_config_properties_get_request_header_t *) data;
> +
> +		int propslen = request_size - G_STRUCT_OFFSET(cfs_guest_config_properties_get_request_header_t, props);
> +
> +		result = 0;
> +		if (rh->vmid < 100 && rh->vmid != 0) {
> +			cfs_debug("vmid out of range %u", rh->vmid);
> +			result = -EINVAL;

sanity check for num_props being > 0 here would probably be good. with 
the current code it's not problematic, but who knows how this evolves in 
the future ;)

(alternatively, we could also drop num_props entirely and just say the 
msg must at most contain 255 properties, and always allocate 255 slots 
below and abort on the 256th property? but I guess the safeguard against 
bugs on the caller side doesn't hurt and it saves a bit of memory in the 
common case which is probably with <10 properties even if we find a few 
more use cases :))

> +		} else if (rh->vmid >= 100 && !vmlist_vm_exists(rh->vmid)) {
> +			result = -ENOENT;
> +		} else if (propslen <= 1) {
> +			cfs_debug("propslen <= 1, %d", propslen);
> +			result = -EINVAL;
> +		} else {
> +			const char **properties = malloc(sizeof(char*) * rh->num_props);

malloc(0), so we must never dereference the returned pointer in that 
case.

> +			char *current = (rh->props);
> +			int remaining = propslen;
> +			for (uint8_t i = 0; i < rh->num_props; i++) {

loop is skipped since 0 < 0 is false

> +			    uint8_t proplen = strnlen(current, remaining);
> +			    if (proplen == 0) {
> +				cfs_debug("property length 0");
> +				result = -EINVAL;
> +				break;
> +			    }
> +			    if (proplen == remaining) {
> +				cfs_debug("property not \\0 terminated");
> +				result = -EINVAL;
> +				break;
> +			    }
> +			    if (current[0] < 'a' || current[0] > 'z') {
> +				cfs_debug("property does not start with [a-z]");
> +				result = -EINVAL;
> +				break;
> +			    }
> +			    properties[i] = current;
> +			    current[proplen] = '\0'; // ensure property is 0 terminated
> +			    remaining -= (proplen + 1);
> +			    current += proplen + 1;
> +			}
> +
> +			if (remaining != 0) {

this must then be true, since we had data but didn't process it in any 
way

> +			    cfs_debug("leftover data after parsing %ul properties", rh->num_props);
> +			    result = -EINVAL;
> +			}
> +
> +			if (result == 0) {
> +			    cfs_debug("cfs_get_guest_config_properties: basic valid checked, do request");
> +			    result = cfs_create_guest_conf_properties_msg(outbuf, memdb, properties, rh->num_props, rh->vmid);
> +			}
> +
> +			free(properties);

so we free it here without ever touching it and return an error. since 
about the only thing we are allowed to do with the result of malloc(0) 
is freeing it, right now this is okay :)

> +		}
>  	} else if (request_id == CFS_IPC_VERIFY_TOKEN) {
>  
>  		cfs_verify_token_request_header_t *rh = (cfs_verify_token_request_header_t *) data;

[..]