tests: Use freecon in testSELinuxCheckLabels

[libvirt PATCH] tests: Use freecon in testSELinuxCheckLabels

Posted by Jiri Denemark 2 years, 2 months ago

This partially reverts commit 0fc4a43d248b86fd54ad7323beb66faec8c1043c.

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
---
 tests/securityselinuxlabeltest.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c
index dfe9d38d34..83532d8090 100644
--- a/tests/securityselinuxlabeltest.c
+++ b/tests/securityselinuxlabeltest.c
@@ -230,7 +230,7 @@ testSELinuxCheckLabels(testSELinuxFile *files, size_t nfiles)
     size_t i;
 
     for (i = 0; i < nfiles; i++) {
-        g_autofree char *ctx = NULL;
+        char *ctx = NULL;
         if (getfilecon(files[i].file, &ctx) < 0) {
             if (errno == ENODATA) {
                 /* nothing to do */
@@ -247,8 +247,10 @@ testSELinuxCheckLabels(testSELinuxFile *files, size_t nfiles)
             virReportError(VIR_ERR_INTERNAL_ERROR,
                            "File %s context '%s' did not match expected '%s'",
                            files[i].file, ctx, files[i].context);
+            freecon(ctx);
             return -1;
         }
+        freecon(ctx);
     }
     return 0;
 }
-- 
2.35.0

Re: [libvirt PATCH] tests: Use freecon in testSELinuxCheckLabels

Posted by Jiri Denemark 2 years, 2 months ago

On Fri, Feb 11, 2022 at 14:34:04 +0100, Jiri Denemark wrote:
> This partially reverts commit 0fc4a43d248b86fd54ad7323beb66faec8c1043c.
> 
> Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
> ---
>  tests/securityselinuxlabeltest.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c
> index dfe9d38d34..83532d8090 100644
> --- a/tests/securityselinuxlabeltest.c
> +++ b/tests/securityselinuxlabeltest.c
> @@ -230,7 +230,7 @@ testSELinuxCheckLabels(testSELinuxFile *files, size_t nfiles)
>      size_t i;
>  
>      for (i = 0; i < nfiles; i++) {
> -        g_autofree char *ctx = NULL;
> +        char *ctx = NULL;
>          if (getfilecon(files[i].file, &ctx) < 0) {
>              if (errno == ENODATA) {
>                  /* nothing to do */
> @@ -247,8 +247,10 @@ testSELinuxCheckLabels(testSELinuxFile *files, size_t nfiles)
>              virReportError(VIR_ERR_INTERNAL_ERROR,
>                             "File %s context '%s' did not match expected '%s'",
>                             files[i].file, ctx, files[i].context);
> +            freecon(ctx);
>              return -1;
>          }
> +        freecon(ctx);
>      }
>      return 0;
>  }

Self-NACK

getfilecon is mocked in this test so not using freecon() was actually
correct.

Re: [libvirt PATCH] tests: Use freecon in testSELinuxCheckLabels

Posted by Ján Tomko 2 years, 2 months ago

On a Friday in 2022, Jiri Denemark wrote:
>On Fri, Feb 11, 2022 at 14:34:04 +0100, Jiri Denemark wrote:
>> This partially reverts commit 0fc4a43d248b86fd54ad7323beb66faec8c1043c.
>>
>> Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
>> ---
>>  tests/securityselinuxlabeltest.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c
>> index dfe9d38d34..83532d8090 100644
>> --- a/tests/securityselinuxlabeltest.c
>> +++ b/tests/securityselinuxlabeltest.c
>> @@ -247,8 +247,10 @@ testSELinuxCheckLabels(testSELinuxFile *files, size_t nfiles)
>>              virReportError(VIR_ERR_INTERNAL_ERROR,
>>                             "File %s context '%s' did not match expected '%s'",
>>                             files[i].file, ctx, files[i].context);
>> +            freecon(ctx);
>>              return -1;
>>          }
>> +        freecon(ctx);
>>      }
>>      return 0;
>>  }
>
>Self-NACK
>
>getfilecon is mocked in this test so not using freecon() was actually
>correct.
>

Even if it was from libselinux [0], is there something wrong with using
plain free for a char * variable?

We seem to have exactly one user of xmlFree which is a similar function.

Jano


[0] https://github.com/SELinuxProject/selinux/blob/master/libselinux/src/freecon.c#L6

Re: [libvirt PATCH] tests: Use freecon in testSELinuxCheckLabels

Posted by Jiri Denemark 2 years, 2 months ago

On Fri, Feb 11, 2022 at 15:16:35 +0100, Ján Tomko wrote:
> On a Friday in 2022, Jiri Denemark wrote:
> >On Fri, Feb 11, 2022 at 14:34:04 +0100, Jiri Denemark wrote:
> >> This partially reverts commit 0fc4a43d248b86fd54ad7323beb66faec8c1043c.
> >>
> >> Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
> >> ---
> >>  tests/securityselinuxlabeltest.c | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c
> >> index dfe9d38d34..83532d8090 100644
> >> --- a/tests/securityselinuxlabeltest.c
> >> +++ b/tests/securityselinuxlabeltest.c
> >> @@ -247,8 +247,10 @@ testSELinuxCheckLabels(testSELinuxFile *files, size_t nfiles)
> >>              virReportError(VIR_ERR_INTERNAL_ERROR,
> >>                             "File %s context '%s' did not match expected '%s'",
> >>                             files[i].file, ctx, files[i].context);
> >> +            freecon(ctx);
> >>              return -1;
> >>          }
> >> +        freecon(ctx);
> >>      }
> >>      return 0;
> >>  }
> >
> >Self-NACK
> >
> >getfilecon is mocked in this test so not using freecon() was actually
> >correct.
> >
> 
> Even if it was from libselinux [0], is there something wrong with using
> plain free for a char * variable?
> 
> We seem to have exactly one user of xmlFree which is a similar function.
> 
> [0] https://github.com/SELinuxProject/selinux/blob/master/libselinux/src/freecon.c#L6

Well, not really, unless the library decides to use a different
allocator which it easily can since the documentation says you should be
using freecon(). And we use freecon in other places in our code to
comply with this.

Anyway, I really wish libraries did not invent their own free functions
for char *.

Jirka

Re: [libvirt PATCH] tests: Use freecon in testSELinuxCheckLabels

Posted by Daniel P. Berrangé 2 years, 2 months ago

On Fri, Feb 11, 2022 at 03:16:35PM +0100, Ján Tomko wrote:
> On a Friday in 2022, Jiri Denemark wrote:
> > On Fri, Feb 11, 2022 at 14:34:04 +0100, Jiri Denemark wrote:
> > > This partially reverts commit 0fc4a43d248b86fd54ad7323beb66faec8c1043c.
> > > 
> > > Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
> > > ---
> > >  tests/securityselinuxlabeltest.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c
> > > index dfe9d38d34..83532d8090 100644
> > > --- a/tests/securityselinuxlabeltest.c
> > > +++ b/tests/securityselinuxlabeltest.c
> > > @@ -247,8 +247,10 @@ testSELinuxCheckLabels(testSELinuxFile *files, size_t nfiles)
> > >              virReportError(VIR_ERR_INTERNAL_ERROR,
> > >                             "File %s context '%s' did not match expected '%s'",
> > >                             files[i].file, ctx, files[i].context);
> > > +            freecon(ctx);
> > >              return -1;
> > >          }
> > > +        freecon(ctx);
> > >      }
> > >      return 0;
> > >  }
> > 
> > Self-NACK
> > 
> > getfilecon is mocked in this test so not using freecon() was actually
> > correct.
> > 
> 
> Even if it was from libselinux [0], is there something wrong with using
> plain free for a char * variable?

SELinux uses to use 'security_context_t' data type for all its
APIs, whicih was a typedef for 'char *'.  They stopped doing
that and now just use 'char *'. The docs for freecon still say
it is to be used to free memory from anmy other get* function,
but impl is just delegating to free()

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 :|