[libvirt] [PATCH] lxc: Clang is complaining about possible NULL pointer.

Julio Faracco posted 1 patch 5 years, 4 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20181105185814.31493-1-jcfaracco@gmail.com
Test syntax-check passed
There is a newer version of this series
src/lxc/lxc_container.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
[libvirt] [PATCH] lxc: Clang is complaining about possible NULL pointer.
Posted by Julio Faracco 5 years, 4 months ago
The array "mount" inside lxc_container is not being checked before for
loop. Clang syntax scan is complaining about this segmentation fault.

Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
---
 src/lxc/lxc_container.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index 918194dacd..6b7bcd8eb6 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -871,7 +871,7 @@ static int lxcContainerSetReadOnly(void)
         qsort(mounts, nmounts, sizeof(mounts[0]),
               virStringSortRevCompare);
 
-    for (i = 0; i < nmounts; i++) {
+    for (i = 0; i < nmounts && mounts; i++) {
         VIR_DEBUG("Bind readonly %s", mounts[i]);
         if (mount(mounts[i], mounts[i], "none", MS_BIND|MS_REC|MS_RDONLY|MS_REMOUNT, NULL) < 0) {
             virReportSystemError(errno,
@@ -883,7 +883,7 @@ static int lxcContainerSetReadOnly(void)
 
     ret = 0;
  cleanup:
-    for (i = 0; i < nmounts; i++)
+    for (i = 0; i < nmounts && mounts; i++)
         VIR_FREE(mounts[i]);
     VIR_FREE(mounts);
     endmntent(procmnt);
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] lxc: Clang is complaining about possible NULL pointer.
Posted by Pino Toscano 5 years, 4 months ago
On Monday, 5 November 2018 19:58:14 CET Julio Faracco wrote:
> The array "mount" inside lxc_container is not being checked before for
> loop. Clang syntax scan is complaining about this segmentation fault.
> 
> Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
> ---
>  src/lxc/lxc_container.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
> index 918194dacd..6b7bcd8eb6 100644
> --- a/src/lxc/lxc_container.c
> +++ b/src/lxc/lxc_container.c
> @@ -871,7 +871,7 @@ static int lxcContainerSetReadOnly(void)
>          qsort(mounts, nmounts, sizeof(mounts[0]),
>                virStringSortRevCompare);

The code here is:

    if (mounts)
        qsort(mounts, nmounts, sizeof(mounts[0]),
              virStringSortRevCompare);

Hence ...

> -    for (i = 0; i < nmounts; i++) {
> +    for (i = 0; i < nmounts && mounts; i++) {
>          VIR_DEBUG("Bind readonly %s", mounts[i]);
>          if (mount(mounts[i], mounts[i], "none", MS_BIND|MS_REC|MS_RDONLY|MS_REMOUNT, NULL) < 0) {
>              virReportSystemError(errno,

... IMHO it is a better idea to move the whole for loop inside the
'if (mounts)' above.

> @@ -883,7 +883,7 @@ static int lxcContainerSetReadOnly(void)
>  
>      ret = 0;
>   cleanup:
> -    for (i = 0; i < nmounts; i++)
> +    for (i = 0; i < nmounts && mounts; i++)
>          VIR_FREE(mounts[i]);

Same idea here: just use a simple if to detect when 'mounts' is non-null.

-- 
Pino Toscano--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] lxc: Clang is complaining about possible NULL pointer.
Posted by Erik Skultety 5 years, 4 months ago
On Mon, Nov 05, 2018 at 04:58:14PM -0200, Julio Faracco wrote:
> The array "mount" inside lxc_container is not being checked before for
> loop. Clang syntax scan is complaining about this segmentation fault.

Which in this specific case is a false positive since nmounts pretty much
protects us from that happening. Usually I'm not very keen on making changes to
silence false positives from analyzers like coverity, however, in this case we
can actually make things better I believe.

>
> Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
> ---
>  src/lxc/lxc_container.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
> index 918194dacd..6b7bcd8eb6 100644
> --- a/src/lxc/lxc_container.c
> +++ b/src/lxc/lxc_container.c
> @@ -871,7 +871,7 @@ static int lxcContainerSetReadOnly(void)

How about we revert the logic HERE (in the missing context) and instead do:

if (!mounts) {
    ret = 0;
    goto cleanup;
}

>          qsort(mounts, nmounts, sizeof(mounts[0]),
>                virStringSortRevCompare);
>
> -    for (i = 0; i < nmounts; i++) {
> +    for (i = 0; i < nmounts && mounts; i++) {
>          VIR_DEBUG("Bind readonly %s", mounts[i]);
>          if (mount(mounts[i], mounts[i], "none", MS_BIND|MS_REC|MS_RDONLY|MS_REMOUNT, NULL) < 0) {
>              virReportSystemError(errno,
> @@ -883,7 +883,7 @@ static int lxcContainerSetReadOnly(void)
>
>      ret = 0;
>   cleanup:
> -    for (i = 0; i < nmounts; i++)
> +    for (i = 0; i < nmounts && mounts; i++)

and then ^here replace this hunk with virStringListFreeCount which already
checks @src for NULL, so that should suffice from Clang's perspective, right?

Erik

>          VIR_FREE(mounts[i]);
>      VIR_FREE(mounts);
>      endmntent(procmnt);
> --
> 2.17.1
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list