[libvirt] [PATCH 0/4] remote: Enforce RPC limits when encoding typed params
Couple of bug fixes here. Michal Prívozník (4): remote_daemon_dispatch: Check for limit properly in remoteDispatchConnectGetAllDomainStats remote: Check for limits when encoding typed params remote_daemon_dispatch: Don't leak @ret on failure remote_daemon_dispatch: Don't open code xdr_free() src/admin/admin_remote.c | 2 + src/admin/admin_server_dispatch.c | 27 ++-------- src/remote/remote_daemon_dispatch.c | 81 ++++++++++++----------------- src/remote/remote_driver.c | 48 +++-------------- src/rpc/gendispatch.pl | 2 + src/util/virtypedparam.c | 13 ++++- src/util/virtypedparam.h | 1 + 7 files changed, 59 insertions(+), 115 deletions(-) -- 2.21.0
The return structure is a bit complicated and that's why it is very easy to check for RPC limits incorrectly. The structure is an array of remote_domain_stats_record structures with the limit of REMOTE_DOMAIN_LIST_MAX. The latter structure then poses a different limit on typed params: REMOTE_CONNECT_GET_ALL_DOMAIN_STATS_MAX (which is what we are checking for mistakenly). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/remote/remote_daemon_dispatch.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index fb8b95f315..1a002957ef 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -6996,15 +6996,15 @@ remoteDispatchConnectGetAllDomainStats(virNetServerPtr server ATTRIBUTE_UNUSED, goto cleanup; } - if (nrecords > REMOTE_CONNECT_GET_ALL_DOMAIN_STATS_MAX) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Number of domain stats records is %d, " - "which exceeds max limit: %d"), - nrecords, REMOTE_CONNECT_GET_ALL_DOMAIN_STATS_MAX); - goto cleanup; - } - if (nrecords) { + if (nrecords > REMOTE_DOMAIN_LIST_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Number of domain stats records is %d, " + "which exceeds max limit: %d"), + nrecords, REMOTE_DOMAIN_LIST_MAX); + goto cleanup; + } + if (VIR_ALLOC_N(ret->retStats.retStats_val, nrecords) < 0) goto cleanup; -- 2.21.0
Looks correct to me. Good catch. Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com> On Tue, 2019-08-27 at 14:05 +0200, Michal Privoznik wrote:
The return structure is a bit complicated and that's why it is very easy to check for RPC limits incorrectly. The structure is an array of remote_domain_stats_record structures with the limit of REMOTE_DOMAIN_LIST_MAX. The latter structure then poses a different limit on typed params: REMOTE_CONNECT_GET_ALL_DOMAIN_STATS_MAX (which is what we are checking for mistakenly).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/remote/remote_daemon_dispatch.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index fb8b95f315..1a002957ef 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -6996,15 +6996,15 @@ remoteDispatchConnectGetAllDomainStats(virNetServerPtr server ATTRIBUTE_UNUSED, goto cleanup; }
- if (nrecords > REMOTE_CONNECT_GET_ALL_DOMAIN_STATS_MAX) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Number of domain stats records is %d, " - "which exceeds max limit: %d"), - nrecords, REMOTE_CONNECT_GET_ALL_DOMAIN_STATS_MAX); - goto cleanup; - } - if (nrecords) { + if (nrecords > REMOTE_DOMAIN_LIST_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Number of domain stats records is %d, " + "which exceeds max limit: %d"), + nrecords, REMOTE_DOMAIN_LIST_MAX); + goto cleanup; + } + if (VIR_ALLOC_N(ret->retStats.retStats_val, nrecords) < 0) goto cleanup;
The same way we check for limits when decoding typed parameters (virTypedParamsDeserialize()) we should do the same check when serializing them so that we don't put onto the wire more than our limits allow. Surprisingly, we were doing so explicitly in some places but not all of them. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/admin/admin_remote.c | 2 ++ src/admin/admin_server_dispatch.c | 27 ++------------- src/remote/remote_daemon_dispatch.c | 54 ++++++++++------------------- src/remote/remote_driver.c | 48 ++++--------------------- src/rpc/gendispatch.pl | 2 ++ src/util/virtypedparam.c | 13 ++++++- src/util/virtypedparam.h | 1 + 7 files changed, 45 insertions(+), 102 deletions(-) diff --git a/src/admin/admin_remote.c b/src/admin/admin_remote.c index e112c1f115..0846e72f6d 100644 --- a/src/admin/admin_remote.c +++ b/src/admin/admin_remote.c @@ -294,6 +294,7 @@ remoteAdminServerSetThreadPoolParameters(virAdmServerPtr srv, virObjectLock(priv); if (virTypedParamsSerialize(params, nparams, + ADMIN_SERVER_THREADPOOL_PARAMETERS_MAX, (virTypedParameterRemotePtr *) &args.params.params_val, &args.params.params_len, 0) < 0) @@ -405,6 +406,7 @@ remoteAdminServerSetClientLimits(virAdmServerPtr srv, virObjectLock(priv); if (virTypedParamsSerialize(params, nparams, + ADMIN_SERVER_CLIENT_LIMITS_MAX, (virTypedParameterRemotePtr *) &args.params.params_val, &args.params.params_len, 0) < 0) diff --git a/src/admin/admin_server_dispatch.c b/src/admin/admin_server_dispatch.c index 1973664488..3c4d72dedb 100644 --- a/src/admin/admin_server_dispatch.c +++ b/src/admin/admin_server_dispatch.c @@ -237,15 +237,8 @@ adminDispatchServerGetThreadpoolParameters(virNetServerPtr server ATTRIBUTE_UNUS args->flags) < 0) goto cleanup; - if (nparams > ADMIN_SERVER_THREADPOOL_PARAMETERS_MAX) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Number of threadpool parameters %d exceeds max " - "allowed limit: %d"), nparams, - ADMIN_SERVER_THREADPOOL_PARAMETERS_MAX); - goto cleanup; - } - if (virTypedParamsSerialize(params, nparams, + ADMIN_SERVER_THREADPOOL_PARAMETERS_MAX, (virTypedParameterRemotePtr *) &ret->params.params_val, &ret->params.params_len, 0) < 0) goto cleanup; @@ -336,15 +329,8 @@ adminDispatchClientGetInfo(virNetServerPtr server ATTRIBUTE_UNUSED, if (adminClientGetInfo(clnt, ¶ms, &nparams, args->flags) < 0) goto cleanup; - if (nparams > ADMIN_CLIENT_INFO_PARAMETERS_MAX) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Number of client info parameters %d exceeds max " - "allowed limit: %d"), nparams, - ADMIN_CLIENT_INFO_PARAMETERS_MAX); - goto cleanup; - } - if (virTypedParamsSerialize(params, nparams, + ADMIN_CLIENT_INFO_PARAMETERS_MAX, (virTypedParameterRemotePtr *) &ret->params.params_val, &ret->params.params_len, VIR_TYPED_PARAM_STRING_OKAY) < 0) @@ -383,15 +369,8 @@ adminDispatchServerGetClientLimits(virNetServerPtr server ATTRIBUTE_UNUSED, if (adminServerGetClientLimits(srv, ¶ms, &nparams, args->flags) < 0) goto cleanup; - if (nparams > ADMIN_SERVER_CLIENT_LIMITS_MAX) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Number of client processing parameters %d exceeds " - "max allowed limit: %d"), nparams, - ADMIN_SERVER_CLIENT_LIMITS_MAX); - goto cleanup; - } - if (virTypedParamsSerialize(params, nparams, + ADMIN_SERVER_CLIENT_LIMITS_MAX, (virTypedParameterRemotePtr *) &ret->params.params_val, &ret->params.params_len, 0) < 0) goto cleanup; diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index 1a002957ef..e205c743c3 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -1152,6 +1152,7 @@ remoteRelayDomainEventTunable(virConnectPtr conn, goto error; if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_EVENT_TUNABLE_MAX, (virTypedParameterRemotePtr *) &data.params.params_val, &data.params.params_len, VIR_TYPED_PARAM_STRING_OKAY) < 0) { @@ -1318,6 +1319,7 @@ remoteRelayDomainEventJobCompleted(virConnectPtr conn, goto error; if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_JOB_STATS_MAX, (virTypedParameterRemotePtr *) &data.params.params_val, &data.params.params_len, VIR_TYPED_PARAM_STRING_OKAY) < 0) { @@ -2450,6 +2452,7 @@ remoteDispatchDomainGetSchedulerParameters(virNetServerPtr server ATTRIBUTE_UNUS goto cleanup; if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_SCHEDULER_PARAMETERS_MAX, (virTypedParameterRemotePtr *) &ret->params.params_val, &ret->params.params_len, 0) < 0) @@ -2498,6 +2501,7 @@ remoteDispatchDomainGetSchedulerParametersFlags(virNetServerPtr server ATTRIBUTE goto cleanup; if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_SCHEDULER_PARAMETERS_MAX, (virTypedParameterRemotePtr *) &ret->params.params_val, &ret->params.params_len, args->flags) < 0) @@ -2663,6 +2667,7 @@ remoteDispatchDomainBlockStatsFlags(virNetServerPtr server ATTRIBUTE_UNUSED, /* Serialize the block stats. */ if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_BLOCK_STATS_PARAMETERS_MAX, (virTypedParameterRemotePtr *) &ret->params.params_val, &ret->params.params_len, args->flags) < 0) @@ -3292,6 +3297,7 @@ remoteDispatchDomainGetMemoryParameters(virNetServerPtr server ATTRIBUTE_UNUSED, } if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_MEMORY_PARAMETERS_MAX, (virTypedParameterRemotePtr *) &ret->params.params_val, &ret->params.params_len, args->flags) < 0) @@ -3351,6 +3357,7 @@ remoteDispatchDomainGetNumaParameters(virNetServerPtr server ATTRIBUTE_UNUSED, } if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_NUMA_PARAMETERS_MAX, (virTypedParameterRemotePtr *) &ret->params.params_val, &ret->params.params_len, flags) < 0) @@ -3410,6 +3417,7 @@ remoteDispatchDomainGetBlkioParameters(virNetServerPtr server ATTRIBUTE_UNUSED, } if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_BLKIO_PARAMETERS_MAX, (virTypedParameterRemotePtr *) &ret->params.params_val, &ret->params.params_len, args->flags) < 0) @@ -3587,12 +3595,8 @@ remoteDispatchDomainGetLaunchSecurityInfo(virNetServerPtr server ATTRIBUTE_UNUSE if (virDomainGetLaunchSecurityInfo(dom, ¶ms, &nparams, args->flags) < 0) goto cleanup; - if (nparams > REMOTE_DOMAIN_LAUNCH_SECURITY_INFO_PARAMS_MAX) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); - goto cleanup; - } - if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_LAUNCH_SECURITY_INFO_PARAMS_MAX, (virTypedParameterRemotePtr *) &ret->params.params_val, &ret->params.params_len, args->flags) < 0) @@ -3631,12 +3635,8 @@ remoteDispatchDomainGetPerfEvents(virNetServerPtr server ATTRIBUTE_UNUSED, if (virDomainGetPerfEvents(dom, ¶ms, &nparams, args->flags) < 0) goto cleanup; - if (nparams > REMOTE_DOMAIN_PERF_EVENTS_MAX) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); - goto cleanup; - } - if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_PERF_EVENTS_MAX, (virTypedParameterRemotePtr *) &ret->params.params_val, &ret->params.params_len, 0) < 0) @@ -3732,6 +3732,7 @@ remoteDispatchDomainGetBlockIoTune(virNetServerPtr server ATTRIBUTE_UNUSED, /* Serialize the block I/O tuning parameters. */ if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_BLOCK_IO_TUNE_PARAMETERS_MAX, (virTypedParameterRemotePtr *) &ret->params.params_val, &ret->params.params_len, args->flags) < 0) @@ -5289,6 +5290,7 @@ remoteDispatchDomainGetInterfaceParameters(virNetServerPtr server ATTRIBUTE_UNUS } if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_INTERFACE_PARAMETERS_MAX, (virTypedParameterRemotePtr *) &ret->params.params_val, &ret->params.params_len, flags) < 0) @@ -5348,6 +5350,7 @@ remoteDispatchDomainGetCPUStats(virNetServerPtr server ATTRIBUTE_UNUSED, goto success; if (virTypedParamsSerialize(params, args->nparams * args->ncpus, + REMOTE_DOMAIN_GET_CPU_STATS_MAX, (virTypedParameterRemotePtr *) &ret->params.params_val, &ret->params.params_len, args->flags) < 0) @@ -5450,13 +5453,8 @@ remoteDispatchNodeGetSevInfo(virNetServerPtr server ATTRIBUTE_UNUSED, if (virNodeGetSEVInfo(conn, ¶ms, &nparams, args->flags) < 0) goto cleanup; - if (nparams > REMOTE_NODE_SEV_INFO_MAX) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); - goto cleanup; - } - - if (virTypedParamsSerialize(params, nparams, + REMOTE_NODE_SEV_INFO_MAX, (virTypedParameterRemotePtr *) &ret->params.params_val, &ret->params.params_len, args->flags) < 0) @@ -5511,6 +5509,7 @@ remoteDispatchNodeGetMemoryParameters(virNetServerPtr server ATTRIBUTE_UNUSED, } if (virTypedParamsSerialize(params, nparams, + REMOTE_NODE_MEMORY_PARAMETERS_MAX, (virTypedParameterRemotePtr *) &ret->params.params_val, &ret->params.params_len, args->flags) < 0) @@ -5641,14 +5640,8 @@ remoteDispatchDomainGetJobStats(virNetServerPtr server ATTRIBUTE_UNUSED, &nparams, args->flags) < 0) goto cleanup; - if (nparams > REMOTE_DOMAIN_JOB_STATS_MAX) { - virReportError(VIR_ERR_RPC, - _("Too many job stats '%d' for limit '%d'"), - nparams, REMOTE_DOMAIN_JOB_STATS_MAX); - goto cleanup; - } - if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_JOB_STATS_MAX, (virTypedParameterRemotePtr *) &ret->params.params_val, &ret->params.params_len, 0) < 0) @@ -7018,6 +7011,7 @@ remoteDispatchConnectGetAllDomainStats(virNetServerPtr server ATTRIBUTE_UNUSED, if (virTypedParamsSerialize(retStats[i]->params, retStats[i]->nparams, + REMOTE_CONNECT_GET_ALL_DOMAIN_STATS_MAX, (virTypedParameterRemotePtr *) &dst->params.params_val, &dst->params.params_len, VIR_TYPED_PARAM_STRING_OKAY) < 0) @@ -7370,12 +7364,8 @@ remoteDispatchNetworkPortGetParameters(virNetServerPtr server ATTRIBUTE_UNUSED, if (virNetworkPortGetParameters(port, ¶ms, &nparams, args->flags) < 0) goto cleanup; - if (nparams > REMOTE_NETWORK_PORT_PARAMETERS_MAX) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); - goto cleanup; - } - if (virTypedParamsSerialize(params, nparams, + REMOTE_NETWORK_PORT_PARAMETERS_MAX, (virTypedParameterRemotePtr *) &ret->params.params_val, &ret->params.params_len, args->flags) < 0) @@ -7674,14 +7664,8 @@ remoteDispatchDomainGetGuestInfo(virNetServerPtr server ATTRIBUTE_UNUSED, if (virDomainGetGuestInfo(dom, args->types, ¶ms, &nparams, args->flags) < 0) goto cleanup; - if (nparams > REMOTE_DOMAIN_GUEST_INFO_PARAMS_MAX) { - virReportError(VIR_ERR_RPC, - _("Too many params in guestinfo: %d for limit %d"), - nparams, REMOTE_DOMAIN_GUEST_INFO_PARAMS_MAX); - goto cleanup; - } - if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_GUEST_INFO_PARAMS_MAX, (virTypedParameterRemotePtr *) &ret->params.params_val, &ret->params.params_len, VIR_TYPED_PARAM_STRING_OKAY) < 0) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 8cc7ab3663..2b86f55035 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -7109,14 +7109,8 @@ remoteDomainMigrateBegin3Params(virDomainPtr domain, make_nonnull_domain(&args.dom, domain); args.flags = flags; - if (nparams > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { - virReportError(VIR_ERR_RPC, - _("Too many migration parameters '%d' for limit '%d'"), - nparams, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); - goto cleanup; - } - if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX, (virTypedParameterRemotePtr *) &args.params.params_val, &args.params.params_len, VIR_TYPED_PARAM_STRING_OKAY) < 0) { @@ -7177,14 +7171,8 @@ remoteDomainMigratePrepare3Params(virConnectPtr dconn, memset(&args, 0, sizeof(args)); memset(&ret, 0, sizeof(ret)); - if (nparams > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { - virReportError(VIR_ERR_RPC, - _("Too many migration parameters '%d' for limit '%d'"), - nparams, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); - goto cleanup; - } - if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX, (virTypedParameterRemotePtr *) &args.params.params_val, &args.params.params_len, VIR_TYPED_PARAM_STRING_OKAY) < 0) { @@ -7261,18 +7249,12 @@ remoteDomainMigratePrepareTunnel3Params(virConnectPtr dconn, memset(&args, 0, sizeof(args)); memset(&ret, 0, sizeof(ret)); - if (nparams > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { - virReportError(VIR_ERR_RPC, - _("Too many migration parameters '%d' for limit '%d'"), - nparams, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); - goto cleanup; - } - args.cookie_in.cookie_in_val = (char *)cookiein; args.cookie_in.cookie_in_len = cookieinlen; args.flags = flags; if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX, (virTypedParameterRemotePtr *) &args.params.params_val, &args.params.params_len, VIR_TYPED_PARAM_STRING_OKAY) < 0) { @@ -7351,13 +7333,6 @@ remoteDomainMigratePerform3Params(virDomainPtr dom, memset(&args, 0, sizeof(args)); memset(&ret, 0, sizeof(ret)); - if (nparams > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { - virReportError(VIR_ERR_RPC, - _("Too many migration parameters '%d' for limit '%d'"), - nparams, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); - goto cleanup; - } - make_nonnull_domain(&args.dom, dom); args.dconnuri = dconnuri == NULL ? NULL : (char **) &dconnuri; args.cookie_in.cookie_in_val = (char *)cookiein; @@ -7365,6 +7340,7 @@ remoteDomainMigratePerform3Params(virDomainPtr dom, args.flags = flags; if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX, (virTypedParameterRemotePtr *) &args.params.params_val, &args.params.params_len, VIR_TYPED_PARAM_STRING_OKAY) < 0) { @@ -7425,19 +7401,13 @@ remoteDomainMigrateFinish3Params(virConnectPtr dconn, memset(&args, 0, sizeof(args)); memset(&ret, 0, sizeof(ret)); - if (nparams > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { - virReportError(VIR_ERR_RPC, - _("Too many migration parameters '%d' for limit '%d'"), - nparams, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); - goto cleanup; - } - args.cookie_in.cookie_in_val = (char *)cookiein; args.cookie_in.cookie_in_len = cookieinlen; args.flags = flags; args.cancelled = cancelled; if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX, (virTypedParameterRemotePtr *) &args.params.params_val, &args.params.params_len, VIR_TYPED_PARAM_STRING_OKAY) < 0) { @@ -7499,13 +7469,6 @@ remoteDomainMigrateConfirm3Params(virDomainPtr domain, memset(&args, 0, sizeof(args)); - if (nparams > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { - virReportError(VIR_ERR_RPC, - _("Too many migration parameters '%d' for limit '%d'"), - nparams, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); - goto cleanup; - } - make_nonnull_domain(&args.dom, domain); args.cookie_in.cookie_in_len = cookieinlen; args.cookie_in.cookie_in_val = (char *) cookiein; @@ -7513,6 +7476,7 @@ remoteDomainMigrateConfirm3Params(virDomainPtr domain, args.cancelled = cancelled; if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX, (virTypedParameterRemotePtr *) &args.params.params_val, &args.params.params_len, VIR_TYPED_PARAM_STRING_OKAY) < 0) { diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl index ff21834091..9d3cc5b145 100755 --- a/src/rpc/gendispatch.pl +++ b/src/rpc/gendispatch.pl @@ -951,6 +951,7 @@ elsif ($mode eq "server") { splice(@args_list, int($5), 0, "&$1_len"); push(@ret_list, "if (virTypedParamsSerialize($1, $1_len,\n" . + " $2,\n" . " (virTypedParameterRemotePtr *) &ret->$1.$1_val,\n" . " &ret->$1.$1_len,\n" . " VIR_TYPED_PARAM_STRING_OKAY) < 0)\n" . @@ -1436,6 +1437,7 @@ elsif ($mode eq "client") { push(@args_list, "virTypedParameterPtr $1"); push(@args_list, "int n$1"); push(@setters_list2, "if (virTypedParamsSerialize($1, n$1,\n" . + " $2,\n" . " (virTypedParameterRemotePtr *) &args.$1.$1_val,\n" . " &args.$1.$1_len,\n" . " VIR_TYPED_PARAM_STRING_OKAY) < 0) {\n" . diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index 8f23348d97..779714d146 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -1480,12 +1480,15 @@ virTypedParamsDeserialize(virTypedParameterRemotePtr remote_params, * virTypedParamsSerialize: * @params: array of parameters to be serialized and later sent to remote side * @nparams: number of elements in @params + * @limit: user specified maximum limit to @remote_params_len * @remote_params_val: protocol independent remote representation of @params * @remote_params_len: the final number of elements in @remote_params_val * @flags: bitwise-OR of virTypedParameterFlags * * This method serializes typed parameters provided by @params into * @remote_params_val which is the representation actually being sent. + * It also checks, if the @limit imposed by RPC on the maximum number of + * parameters is not exceeded. * * Server side using this method also filters out any string parameters that * must not be returned to older clients and handles possibly sparse arrays @@ -1496,6 +1499,7 @@ virTypedParamsDeserialize(virTypedParameterRemotePtr remote_params, int virTypedParamsSerialize(virTypedParameterPtr params, int nparams, + int limit, virTypedParameterRemotePtr *remote_params_val, unsigned int *remote_params_len, unsigned int flags) @@ -1503,9 +1507,16 @@ virTypedParamsSerialize(virTypedParameterPtr params, size_t i; size_t j; int rv = -1; - virTypedParameterRemotePtr params_val; + virTypedParameterRemotePtr params_val = NULL; int params_len = nparams; + if (nparams > limit) { + virReportError(VIR_ERR_RPC, + _("too many parameters '%d' for limit '%d'"), + nparams, limit); + goto cleanup; + } + if (VIR_ALLOC_N(params_val, nparams) < 0) goto cleanup; diff --git a/src/util/virtypedparam.h b/src/util/virtypedparam.h index ce4e75b389..f9d22b24fb 100644 --- a/src/util/virtypedparam.h +++ b/src/util/virtypedparam.h @@ -113,6 +113,7 @@ int virTypedParamsDeserialize(virTypedParameterRemotePtr remote_params, int virTypedParamsSerialize(virTypedParameterPtr params, int nparams, + int limit, virTypedParameterRemotePtr *remote_params_val, unsigned int *remote_params_len, unsigned int flags); -- 2.21.0
Nice improvement. Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com> On Tue, 2019-08-27 at 14:05 +0200, Michal Privoznik wrote:
The same way we check for limits when decoding typed parameters (virTypedParamsDeserialize()) we should do the same check when serializing them so that we don't put onto the wire more than our limits allow. Surprisingly, we were doing so explicitly in some places but not all of them.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/admin/admin_remote.c | 2 ++ src/admin/admin_server_dispatch.c | 27 ++------------- src/remote/remote_daemon_dispatch.c | 54 ++++++++++----------------- -- src/remote/remote_driver.c | 48 ++++--------------------- src/rpc/gendispatch.pl | 2 ++ src/util/virtypedparam.c | 13 ++++++- src/util/virtypedparam.h | 1 + 7 files changed, 45 insertions(+), 102 deletions(-)
diff --git a/src/admin/admin_remote.c b/src/admin/admin_remote.c index e112c1f115..0846e72f6d 100644 --- a/src/admin/admin_remote.c +++ b/src/admin/admin_remote.c @@ -294,6 +294,7 @@ remoteAdminServerSetThreadPoolParameters(virAdmServerPtr srv, virObjectLock(priv);
if (virTypedParamsSerialize(params, nparams, + ADMIN_SERVER_THREADPOOL_PARAMETERS_M AX, (virTypedParameterRemotePtr *) &args.params.params_val, &args.params.params_len, 0) < 0) @@ -405,6 +406,7 @@ remoteAdminServerSetClientLimits(virAdmServerPtr srv, virObjectLock(priv);
if (virTypedParamsSerialize(params, nparams, + ADMIN_SERVER_CLIENT_LIMITS_MAX, (virTypedParameterRemotePtr *) &args.params.params_val, &args.params.params_len, 0) < 0) diff --git a/src/admin/admin_server_dispatch.c b/src/admin/admin_server_dispatch.c index 1973664488..3c4d72dedb 100644 --- a/src/admin/admin_server_dispatch.c +++ b/src/admin/admin_server_dispatch.c @@ -237,15 +237,8 @@ adminDispatchServerGetThreadpoolParameters(virNetServerPtr server ATTRIBUTE_UNUS args->flags) < 0) goto cleanup;
params.params_val, &ret->params.params_len, 0) < 0) goto cleanup; @@ -336,15 +329,8 @@ adminDispatchClientGetInfo(virNetServerPtr server ATTRIBUTE_UNUSED, if (adminClientGetInfo(clnt, ¶ms, &nparams, args->flags) <
- if (nparams > ADMIN_SERVER_THREADPOOL_PARAMETERS_MAX) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Number of threadpool parameters %d exceeds max " - "allowed limit: %d"), nparams, - ADMIN_SERVER_THREADPOOL_PARAMETERS_MAX); - goto cleanup; - } - if (virTypedParamsSerialize(params, nparams, + ADMIN_SERVER_THREADPOOL_PARAMETERS_M AX, (virTypedParameterRemotePtr *) &ret- 0) goto cleanup;
- if (nparams > ADMIN_CLIENT_INFO_PARAMETERS_MAX) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Number of client info parameters %d exceeds max " - "allowed limit: %d"), nparams, - ADMIN_CLIENT_INFO_PARAMETERS_MAX); - goto cleanup; - } - if (virTypedParamsSerialize(params, nparams, + ADMIN_CLIENT_INFO_PARAMETERS_MAX, (virTypedParameterRemotePtr *) &ret-
params.params_val, &ret->params.params_len, VIR_TYPED_PARAM_STRING_OKAY) < 0) @@ -383,15 +369,8 @@ adminDispatchServerGetClientLimits(virNetServerPtr server ATTRIBUTE_UNUSED, if (adminServerGetClientLimits(srv, ¶ms, &nparams, args- flags) < 0) goto cleanup;
- if (nparams > ADMIN_SERVER_CLIENT_LIMITS_MAX) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Number of client processing parameters %d exceeds " - "max allowed limit: %d"), nparams, - ADMIN_SERVER_CLIENT_LIMITS_MAX); - goto cleanup; - } - if (virTypedParamsSerialize(params, nparams, + ADMIN_SERVER_CLIENT_LIMITS_MAX, (virTypedParameterRemotePtr *) &ret-
params.params_val, &ret->params.params_len, 0) < 0) goto cleanup; diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index 1a002957ef..e205c743c3 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -1152,6 +1152,7 @@ remoteRelayDomainEventTunable(virConnectPtr conn, goto error;
if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_EVENT_TUNABLE_MAX, (virTypedParameterRemotePtr *) &data.params.params_val, &data.params.params_len, VIR_TYPED_PARAM_STRING_OKAY) < 0) { @@ -1318,6 +1319,7 @@ remoteRelayDomainEventJobCompleted(virConnectPtr conn, goto error;
if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_JOB_STATS_MAX, (virTypedParameterRemotePtr *) &data.params.params_val, &data.params.params_len, VIR_TYPED_PARAM_STRING_OKAY) < 0) { @@ -2450,6 +2452,7 @@ remoteDispatchDomainGetSchedulerParameters(virNetServerPtr server ATTRIBUTE_UNUS goto cleanup;
if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_SCHEDULER_PARAMETERS_M AX, (virTypedParameterRemotePtr *) &ret-
params.params_val, &ret->params.params_len, 0) < 0) @@ -2498,6 +2501,7 @@ remoteDispatchDomainGetSchedulerParametersFlags(virNetServerPtr server ATTRIBUTE goto cleanup;
if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_SCHEDULER_PARAMETERS_M AX, (virTypedParameterRemotePtr *) &ret-
params.params_val, &ret->params.params_len, args->flags) < 0) @@ -2663,6 +2667,7 @@ remoteDispatchDomainBlockStatsFlags(virNetServerPtr server ATTRIBUTE_UNUSED,
/* Serialize the block stats. */ if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_BLOCK_STATS_PARAMETERS _MAX, (virTypedParameterRemotePtr *) &ret-
params.params_val, &ret->params.params_len, args->flags) < 0) @@ -3292,6 +3297,7 @@ remoteDispatchDomainGetMemoryParameters(virNetServerPtr server ATTRIBUTE_UNUSED, }
if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_MEMORY_PARAMETERS_MAX, (virTypedParameterRemotePtr *) &ret-
params.params_val, &ret->params.params_len, args->flags) < 0) @@ -3351,6 +3357,7 @@ remoteDispatchDomainGetNumaParameters(virNetServerPtr server ATTRIBUTE_UNUSED, }
if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_NUMA_PARAMETERS_MAX, (virTypedParameterRemotePtr *) &ret-
params.params_val, &ret->params.params_len, flags) < 0) @@ -3410,6 +3417,7 @@ remoteDispatchDomainGetBlkioParameters(virNetServerPtr server ATTRIBUTE_UNUSED, }
if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_BLKIO_PARAMETERS_MAX, (virTypedParameterRemotePtr *) &ret-
params.params_val, &ret->params.params_len, args->flags) < 0) @@ -3587,12 +3595,8 @@ remoteDispatchDomainGetLaunchSecurityInfo(virNetServerPtr server ATTRIBUTE_UNUSE if (virDomainGetLaunchSecurityInfo(dom, ¶ms, &nparams, args- flags) < 0) goto cleanup;
- if (nparams > REMOTE_DOMAIN_LAUNCH_SECURITY_INFO_PARAMS_MAX) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); - goto cleanup; - } - if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_LAUNCH_SECURITY_INFO_P ARAMS_MAX, (virTypedParameterRemotePtr *) &ret-
params.params_val, &ret->params.params_len, args->flags) < 0) @@ -3631,12 +3635,8 @@ remoteDispatchDomainGetPerfEvents(virNetServerPtr server ATTRIBUTE_UNUSED, if (virDomainGetPerfEvents(dom, ¶ms, &nparams, args->flags) < 0) goto cleanup;
- if (nparams > REMOTE_DOMAIN_PERF_EVENTS_MAX) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); - goto cleanup; - } - if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_PERF_EVENTS_MAX, (virTypedParameterRemotePtr *) &ret-
params.params_val, &ret->params.params_len, 0) < 0) @@ -3732,6 +3732,7 @@ remoteDispatchDomainGetBlockIoTune(virNetServerPtr server ATTRIBUTE_UNUSED,
/* Serialize the block I/O tuning parameters. */ if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_BLOCK_IO_TUNE_PARAMETE RS_MAX, (virTypedParameterRemotePtr *) &ret-
params.params_val, &ret->params.params_len, args->flags) < 0) @@ -5289,6 +5290,7 @@ remoteDispatchDomainGetInterfaceParameters(virNetServerPtr server ATTRIBUTE_UNUS }
if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_INTERFACE_PARAMETERS_M AX, (virTypedParameterRemotePtr *) &ret-
params.params_val, &ret->params.params_len, flags) < 0) @@ -5348,6 +5350,7 @@ remoteDispatchDomainGetCPUStats(virNetServerPtr server ATTRIBUTE_UNUSED, goto success;
if (virTypedParamsSerialize(params, args->nparams * args->ncpus, + REMOTE_DOMAIN_GET_CPU_STATS_MAX, (virTypedParameterRemotePtr *) &ret-
params.params_val, &ret->params.params_len, args->flags) < 0) @@ -5450,13 +5453,8 @@ remoteDispatchNodeGetSevInfo(virNetServerPtr server ATTRIBUTE_UNUSED, if (virNodeGetSEVInfo(conn, ¶ms, &nparams, args->flags) < 0) goto cleanup;
- if (nparams > REMOTE_NODE_SEV_INFO_MAX) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); - goto cleanup; - } - - if (virTypedParamsSerialize(params, nparams, + REMOTE_NODE_SEV_INFO_MAX, (virTypedParameterRemotePtr *) &ret-
params.params_val, &ret->params.params_len, args->flags) < 0) @@ -5511,6 +5509,7 @@ remoteDispatchNodeGetMemoryParameters(virNetServerPtr server ATTRIBUTE_UNUSED, }
if (virTypedParamsSerialize(params, nparams, + REMOTE_NODE_MEMORY_PARAMETERS_MAX, (virTypedParameterRemotePtr *) &ret-
params.params_val, &ret->params.params_len, args->flags) < 0) @@ -5641,14 +5640,8 @@ remoteDispatchDomainGetJobStats(virNetServerPtr server ATTRIBUTE_UNUSED, &nparams, args->flags) < 0) goto cleanup;
- if (nparams > REMOTE_DOMAIN_JOB_STATS_MAX) { - virReportError(VIR_ERR_RPC, - _("Too many job stats '%d' for limit '%d'"), - nparams, REMOTE_DOMAIN_JOB_STATS_MAX); - goto cleanup; - } - if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_JOB_STATS_MAX, (virTypedParameterRemotePtr *) &ret-
params.params_val, &ret->params.params_len, 0) < 0) @@ -7018,6 +7011,7 @@ remoteDispatchConnectGetAllDomainStats(virNetServerPtr server ATTRIBUTE_UNUSED,
if (virTypedParamsSerialize(retStats[i]->params, retStats[i]->nparams, + REMOTE_CONNECT_GET_ALL_DOMAI N_STATS_MAX, (virTypedParameterRemotePtr *) &dst->params.params_val, &dst->params.params_len, VIR_TYPED_PARAM_STRING_OKAY) < 0) @@ -7370,12 +7364,8 @@ remoteDispatchNetworkPortGetParameters(virNetServerPtr server ATTRIBUTE_UNUSED, if (virNetworkPortGetParameters(port, ¶ms, &nparams, args-
flags) < 0) goto cleanup;
- if (nparams > REMOTE_NETWORK_PORT_PARAMETERS_MAX) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); - goto cleanup; - } - if (virTypedParamsSerialize(params, nparams, + REMOTE_NETWORK_PORT_PARAMETERS_MAX, (virTypedParameterRemotePtr *) &ret-
params.params_val, &ret->params.params_len, args->flags) < 0) @@ -7674,14 +7664,8 @@ remoteDispatchDomainGetGuestInfo(virNetServerPtr server ATTRIBUTE_UNUSED, if (virDomainGetGuestInfo(dom, args->types, ¶ms, &nparams, args->flags) < 0) goto cleanup;
- if (nparams > REMOTE_DOMAIN_GUEST_INFO_PARAMS_MAX) { - virReportError(VIR_ERR_RPC, - _("Too many params in guestinfo: %d for limit %d"), - nparams, REMOTE_DOMAIN_GUEST_INFO_PARAMS_MAX); - goto cleanup; - } - if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_GUEST_INFO_PARAMS_MAX, (virTypedParameterRemotePtr *) &ret-
params.params_val, &ret->params.params_len, VIR_TYPED_PARAM_STRING_OKAY) < 0) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 8cc7ab3663..2b86f55035 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -7109,14 +7109,8 @@ remoteDomainMigrateBegin3Params(virDomainPtr domain, make_nonnull_domain(&args.dom, domain); args.flags = flags;
- if (nparams > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { - virReportError(VIR_ERR_RPC, - _("Too many migration parameters '%d' for limit '%d'"), - nparams, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); - goto cleanup; - } - if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX , (virTypedParameterRemotePtr *) &args.params.params_val, &args.params.params_len, VIR_TYPED_PARAM_STRING_OKAY) < 0) { @@ -7177,14 +7171,8 @@ remoteDomainMigratePrepare3Params(virConnectPtr dconn, memset(&args, 0, sizeof(args)); memset(&ret, 0, sizeof(ret));
- if (nparams > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { - virReportError(VIR_ERR_RPC, - _("Too many migration parameters '%d' for limit '%d'"), - nparams, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); - goto cleanup; - } - if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX , (virTypedParameterRemotePtr *) &args.params.params_val, &args.params.params_len, VIR_TYPED_PARAM_STRING_OKAY) < 0) { @@ -7261,18 +7249,12 @@ remoteDomainMigratePrepareTunnel3Params(virConnectPtr dconn, memset(&args, 0, sizeof(args)); memset(&ret, 0, sizeof(ret));
- if (nparams > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { - virReportError(VIR_ERR_RPC, - _("Too many migration parameters '%d' for limit '%d'"), - nparams, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); - goto cleanup; - } - args.cookie_in.cookie_in_val = (char *)cookiein; args.cookie_in.cookie_in_len = cookieinlen; args.flags = flags;
if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX , (virTypedParameterRemotePtr *) &args.params.params_val, &args.params.params_len, VIR_TYPED_PARAM_STRING_OKAY) < 0) { @@ -7351,13 +7333,6 @@ remoteDomainMigratePerform3Params(virDomainPtr dom, memset(&args, 0, sizeof(args)); memset(&ret, 0, sizeof(ret));
- if (nparams > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { - virReportError(VIR_ERR_RPC, - _("Too many migration parameters '%d' for limit '%d'"), - nparams, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); - goto cleanup; - } - make_nonnull_domain(&args.dom, dom); args.dconnuri = dconnuri == NULL ? NULL : (char **) &dconnuri; args.cookie_in.cookie_in_val = (char *)cookiein; @@ -7365,6 +7340,7 @@ remoteDomainMigratePerform3Params(virDomainPtr dom, args.flags = flags;
if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX , (virTypedParameterRemotePtr *) &args.params.params_val, &args.params.params_len, VIR_TYPED_PARAM_STRING_OKAY) < 0) { @@ -7425,19 +7401,13 @@ remoteDomainMigrateFinish3Params(virConnectPtr dconn, memset(&args, 0, sizeof(args)); memset(&ret, 0, sizeof(ret));
- if (nparams > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { - virReportError(VIR_ERR_RPC, - _("Too many migration parameters '%d' for limit '%d'"), - nparams, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); - goto cleanup; - } - args.cookie_in.cookie_in_val = (char *)cookiein; args.cookie_in.cookie_in_len = cookieinlen; args.flags = flags; args.cancelled = cancelled;
if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX , (virTypedParameterRemotePtr *) &args.params.params_val, &args.params.params_len, VIR_TYPED_PARAM_STRING_OKAY) < 0) { @@ -7499,13 +7469,6 @@ remoteDomainMigrateConfirm3Params(virDomainPtr domain,
memset(&args, 0, sizeof(args));
- if (nparams > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { - virReportError(VIR_ERR_RPC, - _("Too many migration parameters '%d' for limit '%d'"), - nparams, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); - goto cleanup; - } - make_nonnull_domain(&args.dom, domain); args.cookie_in.cookie_in_len = cookieinlen; args.cookie_in.cookie_in_val = (char *) cookiein; @@ -7513,6 +7476,7 @@ remoteDomainMigrateConfirm3Params(virDomainPtr domain, args.cancelled = cancelled;
if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX , (virTypedParameterRemotePtr *) &args.params.params_val, &args.params.params_len, VIR_TYPED_PARAM_STRING_OKAY) < 0) { diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl index ff21834091..9d3cc5b145 100755 --- a/src/rpc/gendispatch.pl +++ b/src/rpc/gendispatch.pl @@ -951,6 +951,7 @@ elsif ($mode eq "server") { splice(@args_list, int($5), 0, "&$1_len");
push(@ret_list, "if (virTypedParamsSerialize($1, $1_len,\n" . + " $2,\n" . " (virTypedParameterRemotePtr *) &ret->$1.$1_val,\n" . " &ret->$1.$1_len,\n" . " VIR_TYPED_PARAM_STRING_OKAY) < 0)\n" . @@ -1436,6 +1437,7 @@ elsif ($mode eq "client") { push(@args_list, "virTypedParameterPtr $1"); push(@args_list, "int n$1"); push(@setters_list2, "if (virTypedParamsSerialize($1, n$1,\n" . + " $2,\n" . " (virTypedParameterRemotePtr *) &args.$1.$1_val,\n" . " &args.$1.$1_len,\n" . " VIR_TYPED_PARAM_STRING_OKAY) < 0) {\n" . diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index 8f23348d97..779714d146 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -1480,12 +1480,15 @@ virTypedParamsDeserialize(virTypedParameterRemotePtr remote_params, * virTypedParamsSerialize: * @params: array of parameters to be serialized and later sent to remote side * @nparams: number of elements in @params + * @limit: user specified maximum limit to @remote_params_len * @remote_params_val: protocol independent remote representation of @params * @remote_params_len: the final number of elements in @remote_params_val * @flags: bitwise-OR of virTypedParameterFlags * * This method serializes typed parameters provided by @params into * @remote_params_val which is the representation actually being sent. + * It also checks, if the @limit imposed by RPC on the maximum number of + * parameters is not exceeded. * * Server side using this method also filters out any string parameters that * must not be returned to older clients and handles possibly sparse arrays @@ -1496,6 +1499,7 @@ virTypedParamsDeserialize(virTypedParameterRemotePtr remote_params, int virTypedParamsSerialize(virTypedParameterPtr params, int nparams, + int limit, virTypedParameterRemotePtr *remote_params_val, unsigned int *remote_params_len, unsigned int flags) @@ -1503,9 +1507,16 @@ virTypedParamsSerialize(virTypedParameterPtr params, size_t i; size_t j; int rv = -1; - virTypedParameterRemotePtr params_val; + virTypedParameterRemotePtr params_val = NULL; int params_len = nparams;
+ if (nparams > limit) { + virReportError(VIR_ERR_RPC, + _("too many parameters '%d' for limit '%d'"), + nparams, limit); + goto cleanup; + } + if (VIR_ALLOC_N(params_val, nparams) < 0) goto cleanup;
diff --git a/src/util/virtypedparam.h b/src/util/virtypedparam.h index ce4e75b389..f9d22b24fb 100644 --- a/src/util/virtypedparam.h +++ b/src/util/virtypedparam.h @@ -113,6 +113,7 @@ int virTypedParamsDeserialize(virTypedParameterRemotePtr remote_params,
int virTypedParamsSerialize(virTypedParameterPtr params, int nparams, + int limit, virTypedParameterRemotePtr *remote_params_val, unsigned int *remote_params_len, unsigned int flags);
If there's a problem in encoding @ret (for instance virTypedParamsSerialize() fails) then @ret is leaked. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/remote/remote_daemon_dispatch.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index e205c743c3..c658a6e115 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -7025,8 +7025,11 @@ remoteDispatchConnectGetAllDomainStats(virNetServerPtr server ATTRIBUTE_UNUSED, rv = 0; cleanup: - if (rv < 0) + if (rv < 0) { virNetMessageSaveError(rerr); + xdr_free((xdrproc_t)xdr_remote_connect_get_all_domain_stats_ret, + (char *) ret); + } virDomainStatsRecordListFree(retStats); virObjectListFree(doms); -- 2.21.0
At two places we are open coding xdr_free(): remoteRelayDomainEventTunable() and remoteRelayDomainEventJobCompleted(). Bot of these functions use make_nonnull_domain() to put domain IDs tuple into return structure and then continue encoding the rest of structure. If that fails, they call VIR_FREE() directly. While this okay, we should use xdr_free() which frees the whole return structure for us. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/remote/remote_daemon_dispatch.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index c658a6e115..ecde959088 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -1156,8 +1156,7 @@ remoteRelayDomainEventTunable(virConnectPtr conn, (virTypedParameterRemotePtr *) &data.params.params_val, &data.params.params_len, VIR_TYPED_PARAM_STRING_OKAY) < 0) { - VIR_FREE(data.dom.name); - return -1; + goto error; } remoteDispatchObjectEventSend(callback->client, remoteProgram, @@ -1323,8 +1322,7 @@ remoteRelayDomainEventJobCompleted(virConnectPtr conn, (virTypedParameterRemotePtr *) &data.params.params_val, &data.params.params_len, VIR_TYPED_PARAM_STRING_OKAY) < 0) { - VIR_FREE(data.dom.name); - return -1; + goto error; } remoteDispatchObjectEventSend(callback->client, remoteProgram, -- 2.21.0
participants (3)
-
Erik Skultety -
Jonathon Jongsma -
Michal Privoznik