10:34 a.m.
Reviewing the sources, I found, that
in function lxcContainerMountAllFS() pointers
vmDef->fss[i]->src and vmDef->fss[i]->src->path
are checked for NULL after dereferencing in
VIR_DEBUG() macro.
Fixes: 57487085dc ("lxc: don't try to reference NULL when mounting
filesystems")
---
src/lxc/lxc_container.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index 21220661f7..58a6695458 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -1467,13 +1467,15 @@ static int lxcContainerMountAllFS(virDomainDef *vmDef,
if (STREQ(vmDef->fss[i]->dst, "/"))
continue;
+ if (!(vmDef->fss[i]->src && vmDef->fss[i]->src->path))
+ return -1;
+
VIR_DEBUG("Mounting '%s' -> '%s'",
vmDef->fss[i]->src->path, vmDef->fss[i]->dst);
if (lxcContainerResolveSymlinks(vmDef->fss[i], false) < 0)
return -1;
- if (!(vmDef->fss[i]->src && vmDef->fss[i]->src->path
&&
- STRPREFIX(vmDef->fss[i]->src->path, vmDef->fss[i]->dst))
&&
+ if (!STRPREFIX(vmDef->fss[i]->src->path, vmDef->fss[i]->dst)
&&
lxcContainerUnmountSubtree(vmDef->fss[i]->dst, false) < 0)
return -1;
--
2.34.1
2:11 a.m.
On Wed, Sep 06, 2023 at 06:34:42PM +0300, Dmitry Frolov wrote:
Reviewing the sources, I found, that
in function lxcContainerMountAllFS() pointers
vmDef->fss[i]->src and vmDef->fss[i]->src->path
are checked for NULL after dereferencing in
VIR_DEBUG() macro.
Fixes: 57487085dc ("lxc: don't try to reference NULL when mounting
filesystems")
---
src/lxc/lxc_container.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index 21220661f7..58a6695458 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -1467,13 +1467,15 @@ static int lxcContainerMountAllFS(virDomainDef *vmDef,
if (STREQ(vmDef->fss[i]->dst, "/"))
continue;
+ if (!(vmDef->fss[i]->src && vmDef->fss[i]->src->path))
+ return -1;
+
This unfortunately changes the semantics. This fails whenever there is
src or src->path missing, but ...
VIR_DEBUG("Mounting '%s' ->
'%s'", vmDef->fss[i]->src->path, vmDef->fss[i]->dst);
if (lxcContainerResolveSymlinks(vmDef->fss[i], false) < 0)
return -1;
- if (!(vmDef->fss[i]->src && vmDef->fss[i]->src->path
&&
- STRPREFIX(vmDef->fss[i]->src->path, vmDef->fss[i]->dst))
&&
+ if (!STRPREFIX(vmDef->fss[i]->src->path, vmDef->fss[i]->dst)
&&
lxcContainerUnmountSubtree(vmDef->fss[i]->dst, false) < 0)
... here it only skips the unmount in such case. That makes sense
because for example tmpfs does not have a source path, only the
destination and the mount that is not visible in the context here would
not only be skipped as well, but the whole starting of a domain would
fail.
Instead of this patch, you could just conditionally change the
VIR_DEBUG() to report what's actually happening.
return -1;
--
2.34.1
2:25 a.m.
On Thu, Sep 07, 2023 at 09:11:34 +0200, Martin Kletzander wrote:
On Wed, Sep 06, 2023 at 06:34:42PM +0300, Dmitry Frolov wrote:
> Reviewing the sources, I found, that
> in function lxcContainerMountAllFS() pointers
> vmDef->fss[i]->src and vmDef->fss[i]->src->path
> are checked for NULL after dereferencing in
> VIR_DEBUG() macro.
>
> Fixes: 57487085dc ("lxc: don't try to reference NULL when mounting
filesystems")
> ---
> src/lxc/lxc_container.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
[...]
> if (lxcContainerResolveSymlinks(vmDef->fss[i], false)
< 0)
> return -1;
>
> - if (!(vmDef->fss[i]->src && vmDef->fss[i]->src->path
&&
> - STRPREFIX(vmDef->fss[i]->src->path,
vmDef->fss[i]->dst)) &&
> + if (!STRPREFIX(vmDef->fss[i]->src->path, vmDef->fss[i]->dst)
&&
> lxcContainerUnmountSubtree(vmDef->fss[i]->dst, false) < 0)
... here it only skips the unmount in such case. That makes sense
because for example tmpfs does not have a source path, only the
destination and the mount that is not visible in the context here would
not only be skipped as well, but the whole starting of a domain would
fail.
Instead of this patch, you could just conditionally change the
VIR_DEBUG() to report what's actually happening.
And we have a handy NULLSTR() macro for that case.
> return -1;
>
> --
> 2.34.1
>
2:35 a.m.
On Thu, Sep 07, 2023 at 09:25:14AM +0200, Peter Krempa wrote:
On Thu, Sep 07, 2023 at 09:11:34 +0200, Martin Kletzander wrote:
> On Wed, Sep 06, 2023 at 06:34:42PM +0300, Dmitry Frolov wrote:
> > Reviewing the sources, I found, that
> > in function lxcContainerMountAllFS() pointers
> > vmDef->fss[i]->src and vmDef->fss[i]->src->path
> > are checked for NULL after dereferencing in
> > VIR_DEBUG() macro.
> >
> > Fixes: 57487085dc ("lxc: don't try to reference NULL when mounting
filesystems")
> > ---
> > src/lxc/lxc_container.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
[...]
> > if (lxcContainerResolveSymlinks(vmDef->fss[i], false) < 0)
> > return -1;
> >
> > - if (!(vmDef->fss[i]->src &&
vmDef->fss[i]->src->path &&
> > - STRPREFIX(vmDef->fss[i]->src->path,
vmDef->fss[i]->dst)) &&
> > + if (!STRPREFIX(vmDef->fss[i]->src->path,
vmDef->fss[i]->dst) &&
> > lxcContainerUnmountSubtree(vmDef->fss[i]->dst, false) < 0)
>
> ... here it only skips the unmount in such case. That makes sense
> because for example tmpfs does not have a source path, only the
> destination and the mount that is not visible in the context here would
> not only be skipped as well, but the whole starting of a domain would
> fail.
>
> Instead of this patch, you could just conditionally change the
> VIR_DEBUG() to report what's actually happening.
And we have a handy NULLSTR() macro for that case.
NULLSTR(a->b) won't help you if a == NULL, unfortunately :(
>
> > return -1;
> >
> > --
> > 2.34.1
> >
2:31 a.m.
On Wed, Sep 06, 2023 at 18:34:42 +0300, Dmitry Frolov wrote:
Reviewing the sources, I found, that
in function lxcContainerMountAllFS() pointers
vmDef->fss[i]->src and vmDef->fss[i]->src->path
are checked for NULL after dereferencing in
VIR_DEBUG() macro.
Fixes: 57487085dc ("lxc: don't try to reference NULL when mounting
filesystems")
---
src/lxc/lxc_container.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index 21220661f7..58a6695458 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -1467,13 +1467,15 @@ static int lxcContainerMountAllFS(virDomainDef *vmDef,
if (STREQ(vmDef->fss[i]->dst, "/"))
continue;
+ if (!(vmDef->fss[i]->src && vmDef->fss[i]->src->path))
+ return -1;
This would return -1 without reporting any error. And you are also
changing the logic here without explaining it. If vmDef->fss[i]->src was
NULL or vmDef->fss[i]->src->path was NULL this function would call
lxcContainerUnmountSubtree on vmDef->fss[i]->dst before, but now it just
returns -1. Well except that it would crash first :-) In any case, this
part is wrong, see below...
+
VIR_DEBUG("Mounting '%s' -> '%s'",
vmDef->fss[i]->src->path, vmDef->fss[i]->dst);
if (lxcContainerResolveSymlinks(vmDef->fss[i], false) < 0)
return -1;
- if (!(vmDef->fss[i]->src && vmDef->fss[i]->src->path
&&
- STRPREFIX(vmDef->fss[i]->src->path, vmDef->fss[i]->dst))
&&
+ if (!STRPREFIX(vmDef->fss[i]->src->path, vmDef->fss[i]->dst)
&&
lxcContainerUnmountSubtree(vmDef->fss[i]->dst, false) < 0)
return -1;
I checked our parsing code and vmDef->fss[i]->src can never be NULL as
it is always allocated in virDomainFSDefNew so checking it is redundant.
On the other hand vmDef->fss[i]->src->path can be NULL, but it is not en
error, e.g., VIR_DOMAIN_FS_TYPE_RAM would contain path == NULL.
So the fix should rather remove the check for vmDef->fss[i]->src and use
NULLSTR(vmDef->fss[i]->src->path) in VIR_DEBUG.
Jirka
443
days inactive
444
days old
4 comments
4 participants
participants (4)
-
Dmitry Frolov -
Jiri Denemark -
Martin Kletzander -
Peter Krempa