[libvirt] [PATCH 0/3] util: storage: fix indexed access to backing store
Improve error reporting when an index can't be found in the backing chain. Peter Krempa (3): util: storage: Fix possible crash when source path is NULL util: storage: Add hint to error message that indexed access was used util: storage: Improve error message when requesting image above 'start' src/util/virstoragefile.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) -- 2.3.5
Some storage protocols allow to have the @path field in struct virStorageSource set to NULL. Add NULLSTR() wrappers to handle this possibility until I finish the storage source error formatter. --- src/util/virstoragefile.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 824cf5d..46aff92 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1391,20 +1391,21 @@ virStorageFileChainLookup(virStorageSourcePtr chain, if (idx) { virReportError(VIR_ERR_INVALID_ARG, _("could not find backing store %u in chain for '%s'"), - idx, start); + idx, NULLSTR(start)); } else if (name) { if (startFrom) virReportError(VIR_ERR_INVALID_ARG, _("could not find image '%s' beneath '%s' in " - "chain for '%s'"), name, startFrom->path, start); + "chain for '%s'"), name, NULLSTR(startFrom->path), + NULLSTR(start)); else virReportError(VIR_ERR_INVALID_ARG, _("could not find image '%s' in chain for '%s'"), - name, start); + name, NULLSTR(start)); } else { virReportError(VIR_ERR_INVALID_ARG, _("could not find base image in chain for '%s'"), - start); + NULLSTR(start)); } *parent = NULL; return NULL; -- 2.3.5
--- src/util/virstoragefile.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 46aff92..c9d3977 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1390,7 +1390,8 @@ virStorageFileChainLookup(virStorageSourcePtr chain, error: if (idx) { virReportError(VIR_ERR_INVALID_ARG, - _("could not find backing store %u in chain for '%s'"), + _("could not find backing store index %u in chain " + "for '%s'"), idx, NULLSTR(start)); } else if (name) { if (startFrom) -- 2.3.5
When a user would specify a backing chain index that is above the start point libvirt would report a rather unhelpful error: invalid argument: could not find backing store 1 in chain for 'sub/link2' This patch adds an explicit check that the index is below start point in the backing store and reports the following error if not: invalid argument: requested backing store index 1 is above 'sub/../qcow2' in chain for 'sub/link2' Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177062 --- src/util/virstoragefile.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index c9d3977..2a2f238 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1341,6 +1341,15 @@ virStorageFileChainLookup(virStorageSourcePtr chain, *parent = startFrom; } + if (idx && idx < i) { + virReportError(VIR_ERR_INVALID_ARG, + _("requested backing store index %u is above '%s' " + "in chain for '%s'"), + idx, NULLSTR(startFrom->path), NULLSTR(start)); + *parent = NULL; + return NULL; + } + while (chain) { if (!name && !idx) { if (!chain->backingStore) -- 2.3.5
On Tue, Apr 21, 2015 at 05:52:14PM +0200, Peter Krempa wrote:
When a user would specify a backing chain index that is above the start point libvirt would report a rather unhelpful error:
invalid argument: could not find backing store 1 in chain for 'sub/link2'
This patch adds an explicit check that the index is below start point in the backing store and reports the following error if not:
invalid argument: requested backing store index 1 is above 'sub/../qcow2' in chain for 'sub/link2'
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177062 --- src/util/virstoragefile.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index c9d3977..2a2f238 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1341,6 +1341,15 @@ virStorageFileChainLookup(virStorageSourcePtr chain, *parent = startFrom; }
+ if (idx && idx < i) { + virReportError(VIR_ERR_INVALID_ARG, + _("requested backing store index %u is above '%s' " + "in chain for '%s'"), + idx, NULLSTR(startFrom->path), NULLSTR(start));
Possible crasher: startFrom can be NULL here, rather move the check into the previous block where the parent is being set when found (idx can't be < i when there is no startFrom anyway). You can then avoid cleaning up the *parent here -+ | +-----------+ | v
+ *parent = NULL; + return NULL; + } +
ACK series with that fixed.
On Wed, Apr 22, 2015 at 14:09:18 +0200, Martin Kletzander wrote:
On Tue, Apr 21, 2015 at 05:52:14PM +0200, Peter Krempa wrote:
When a user would specify a backing chain index that is above the start point libvirt would report a rather unhelpful error:
invalid argument: could not find backing store 1 in chain for 'sub/link2'
This patch adds an explicit check that the index is below start point in the backing store and reports the following error if not:
invalid argument: requested backing store index 1 is above 'sub/../qcow2' in chain for 'sub/link2'
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177062 --- src/util/virstoragefile.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index c9d3977..2a2f238 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1341,6 +1341,15 @@ virStorageFileChainLookup(virStorageSourcePtr chain, *parent = startFrom; }
+ if (idx && idx < i) { + virReportError(VIR_ERR_INVALID_ARG, + _("requested backing store index %u is above '%s' " + "in chain for '%s'"), + idx, NULLSTR(startFrom->path), NULLSTR(start));
Possible crasher: startFrom can be NULL here, rather move the check into the previous block where the parent is being set when found (idx can't be < i when there is no startFrom anyway). You can then avoid cleaning up the *parent here -+ | +-----------+ | v
Actually right at the beginning of the function @parent gets filled by a pointer to a local virStorageSource pointer (stack allocated) in case the caller passed NULL as @parent. This allows to use the variable since it is used in the lookup loop even if the user doesn't pass it. Don't worry, I had the same thought when I wrote the condition :) Peter
On Wed, Apr 22, 2015 at 02:15:21PM +0200, Peter Krempa wrote:
On Wed, Apr 22, 2015 at 14:09:18 +0200, Martin Kletzander wrote:
On Tue, Apr 21, 2015 at 05:52:14PM +0200, Peter Krempa wrote:
When a user would specify a backing chain index that is above the start point libvirt would report a rather unhelpful error:
invalid argument: could not find backing store 1 in chain for 'sub/link2'
This patch adds an explicit check that the index is below start point in the backing store and reports the following error if not:
invalid argument: requested backing store index 1 is above 'sub/../qcow2' in chain for 'sub/link2'
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177062 --- src/util/virstoragefile.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index c9d3977..2a2f238 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1341,6 +1341,15 @@ virStorageFileChainLookup(virStorageSourcePtr chain, *parent = startFrom; }
+ if (idx && idx < i) { + virReportError(VIR_ERR_INVALID_ARG, + _("requested backing store index %u is above '%s' " + "in chain for '%s'"), + idx, NULLSTR(startFrom->path), NULLSTR(start));
Possible crasher: startFrom can be NULL here, rather move the check into the previous block where the parent is being set when found (idx can't be < i when there is no startFrom anyway). You can then avoid cleaning up the *parent here -+ | +-----------+ | v
Actually right at the beginning of the function @parent gets filled by a pointer to a local virStorageSource pointer (stack allocated) in case the caller passed NULL as @parent. This allows to use the variable since it is used in the lookup loop even if the user doesn't pass it.
Yeah, but I was talking about startFrom->path sigsegv-ing if startFrom == NULL.
Don't worry, I had the same thought when I wrote the condition :)
Don't worry, I know people sometime misunderstand me ;)
On Wed, Apr 22, 2015 at 14:24:22 +0200, Martin Kletzander wrote:
On Wed, Apr 22, 2015 at 02:15:21PM +0200, Peter Krempa wrote:
On Wed, Apr 22, 2015 at 14:09:18 +0200, Martin Kletzander wrote:
On Tue, Apr 21, 2015 at 05:52:14PM +0200, Peter Krempa wrote:
When a user would specify a backing chain index that is above the start point libvirt would report a rather unhelpful error:
invalid argument: could not find backing store 1 in chain for 'sub/link2'
This patch adds an explicit check that the index is below start point in the backing store and reports the following error if not:
invalid argument: requested backing store index 1 is above 'sub/../qcow2' in chain for 'sub/link2'
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177062 --- src/util/virstoragefile.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index c9d3977..2a2f238 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1341,6 +1341,15 @@ virStorageFileChainLookup(virStorageSourcePtr chain, *parent = startFrom; }
+ if (idx && idx < i) { + virReportError(VIR_ERR_INVALID_ARG, + _("requested backing store index %u is above '%s' " + "in chain for '%s'"), + idx, NULLSTR(startFrom->path), NULLSTR(start));
Possible crasher: startFrom can be NULL here, rather move the check into the previous block where the parent is being set when found (idx can't be < i when there is no startFrom anyway). You can then avoid cleaning up the *parent here -+ | +-----------+ | v
Actually right at the beginning of the function @parent gets filled by a pointer to a local virStorageSource pointer (stack allocated) in case the caller passed NULL as @parent. This allows to use the variable since it is used in the lookup loop even if the user doesn't pass it.
Yeah, but I was talking about startFrom->path sigsegv-ing if startFrom == NULL.
Don't worry, I had the same thought when I wrote the condition :)
Don't worry, I know people sometime misunderstand me ;)
Okay, after some explaining I understand now what you meant. The misunderstanding was mainly due to my incapacitaded mental power today. I'll move the check as you've suggested and push the series. Thanks. Peter
participants (2)
-
Martin Kletzander -
Peter Krempa