On Wed, Mar 05, 2025 at 03:48:10PM -0700, Jim Fehlig via Devel wrote:
When invoking virDomainSaveParams with a relative path, the image is saved to the daemon's CWD. Similarly, when providing virDomainRestoreParams with a relative path, it attempts to restore from the daemon's CWD. In most configurations, the daemon's CWD is set to '/'. Ensure a relative path is converted to absolute before invoking the driver domain{Save,Restore}Params functions.
Are you aware of any common usage of these APIs with a relative path ? Although we've not documented it, my general view is that all paths given to libvirt are expected to be absolute, everywhere - whether API parameters like these, or config file parmeters, or XML elements/attributes. In the perfect world we would canonicalize all relative paths on the client side, but doing that is such an enourmous & complex job it is not likely to be practical. We'd be better off just documenting use of relative paths as "out of scope" and / or "undefined behaviour"
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libvirt-domain.c | 89 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 73 insertions(+), 16 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 18451ebdb9..0ee7c6f45b 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -1023,6 +1023,11 @@ virDomainSaveParams(virDomainPtr domain, unsigned int flags) { virConnectPtr conn; + virTypedParameterPtr params_copy = NULL; + int nparams_copy = 0; + const char *to = NULL; + g_autofree char *absolute_to = NULL; + int ret = -1;
VIR_DOMAIN_DEBUG(domain, "params=%p, nparams=%d, flags=0x%x", params, nparams, flags); @@ -1033,23 +1038,46 @@ virDomainSaveParams(virDomainPtr domain, virCheckDomainReturn(domain, -1); conn = domain->conn;
- virCheckReadOnlyGoto(conn->flags, error); + virCheckReadOnlyGoto(conn->flags, done);
VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_SAVE_RUNNING, VIR_DOMAIN_SAVE_PAUSED, - error); + done); + + /* We must absolutize the file path as the save is done out of process */ + virTypedParamsCopy(¶ms_copy, params, nparams); + nparams_copy = nparams; + if (virTypedParamsGetString(params_copy, nparams_copy, + VIR_DOMAIN_SAVE_PARAM_FILE, &to) < 0) + goto done; + + if (to) { + if (!(absolute_to = g_canonicalize_filename(to, NULL))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("could not build absolute output file path")); + goto done; + } + + if (virTypedParamsReplaceString(¶ms_copy, &nparams_copy, + VIR_DOMAIN_SAVE_PARAM_FILE, + absolute_to) < 0) + goto done; + }
if (conn->driver->domainSaveParams) { - if (conn->driver->domainSaveParams(domain, params, nparams, flags) < 0) - goto error; - return 0; + if (conn->driver->domainSaveParams(domain, params_copy, nparams_copy, flags) < 0) + goto done; + ret = 0; + } else { + virReportUnsupportedError(); }
- virReportUnsupportedError(); + done: + if (ret < 0) + virDispatchError(domain->conn); + virTypedParamsFree(params_copy, nparams_copy);
- error: - virDispatchError(domain->conn); - return -1; + return ret; }
@@ -1206,6 +1234,12 @@ virDomainRestoreParams(virConnectPtr conn, virTypedParameterPtr params, int nparams, unsigned int flags) { + virTypedParameterPtr params_copy = NULL; + int nparams_copy = 0; + const char *from = NULL; + g_autofree char *absolute_from = NULL; + int ret = -1; + VIR_DEBUG("conn=%p, params=%p, nparams=%d, flags=0x%x", conn, params, nparams, flags); VIR_TYPED_PARAMS_DEBUG(params, nparams); @@ -1213,23 +1247,46 @@ virDomainRestoreParams(virConnectPtr conn, virResetLastError();
virCheckConnectReturn(conn, -1); - virCheckReadOnlyGoto(conn->flags, error); + virCheckReadOnlyGoto(conn->flags, done);
VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_SAVE_RUNNING, VIR_DOMAIN_SAVE_PAUSED, - error); + done);
if (conn->driver->domainRestoreParams) { + /* We must absolutize the file path as the save is done out of process */ + virTypedParamsCopy(¶ms_copy, params, nparams); + nparams_copy = nparams; + if (virTypedParamsGetString(params_copy, nparams_copy, + VIR_DOMAIN_SAVE_PARAM_FILE, &from) < 0) + goto done; + + if (from) { + if (!(absolute_from = g_canonicalize_filename(from, NULL))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("could not build absolute output file path")); + goto done; + } + + if (virTypedParamsReplaceString(¶ms_copy, &nparams_copy, + VIR_DOMAIN_SAVE_PARAM_FILE, + absolute_from) < 0) + goto done; + } + if (conn->driver->domainRestoreParams(conn, params, nparams, flags) < 0) - goto error; - return 0; + goto done; + ret = 0; }
virReportUnsupportedError();
- error: - virDispatchError(conn); - return -1; + done: + if (ret < 0) + virDispatchError(conn); + virTypedParamsFree(params_copy, nparams_copy); + + return ret; }
-- 2.43.0
With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|