1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH] tests: don't abort in fopen(/proc/mounts)

Daniel P. Berrangé posted 1 patch 5 years ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20190327103127.25303-1-berrange@redhat.com
tests/vircgroupmock.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
[libvirt] [PATCH] tests: don't abort in fopen(/proc/mounts)
Posted by Daniel P. Berrangé 5 years ago 
The mock fopen() function will abort if "/proc/mounts" is
requested with "r" permissions and VIR_CGROUP_MOCK_FILENAME
env var is not set.

Unfortunately this is triggering by the libselinux library
constructor when it tries to read /proc/mounts to find out
if selinuxfs is mounted in an unusual place.

This, however, only affects libselinux in Debian as that
opens with "r", while in Fedora / RHEL it opens "re" and
thus luckily never triggered the abort(), instead getting
an EACCESS.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/vircgroupmock.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tests/vircgroupmock.c b/tests/vircgroupmock.c
index 06bd0a5f29..9c67a44b0d 100644
--- a/tests/vircgroupmock.c
+++ b/tests/vircgroupmock.c
@@ -460,8 +460,10 @@ FILE *fopen(const char *path, const char *mode)
     }
 
     if (type) {
-        if (!filename)
-            abort();
+        if (!filename) {
+            errno = EACCES;
+            return NULL;
+        }
         if (virAsprintfQuiet(&filepath, "%s/vircgroupdata/%s.%s",
                              abs_srcdir, filename, type) < 0) {
             abort();
-- 
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tests: don't abort in fopen(/proc/mounts)
Posted by Michal Privoznik 5 years ago 
On 3/27/19 11:31 AM, Daniel P. Berrangé wrote:
> The mock fopen() function will abort if "/proc/mounts" is
> requested with "r" permissions and VIR_CGROUP_MOCK_FILENAME
> env var is not set.
> 
> Unfortunately this is triggering by the libselinux library
> constructor when it tries to read /proc/mounts to find out
> if selinuxfs is mounted in an unusual place.
> 
> This, however, only affects libselinux in Debian as that
> opens with "r", while in Fedora / RHEL it opens "re" and
> thus luckily never triggered the abort(), instead getting
> an EACCESS.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/vircgroupmock.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/vircgroupmock.c b/tests/vircgroupmock.c
> index 06bd0a5f29..9c67a44b0d 100644
> --- a/tests/vircgroupmock.c
> +++ b/tests/vircgroupmock.c
> @@ -460,8 +460,10 @@ FILE *fopen(const char *path, const char *mode)
>       }
>   
>       if (type) {
> -        if (!filename)
> -            abort();
> +        if (!filename) {
> +            errno = EACCES;
> +            return NULL;
> +        }
>           if (virAsprintfQuiet(&filepath, "%s/vircgroupdata/%s.%s",
>                                abs_srcdir, filename, type) < 0) {
>               abort();
> 

How about:

diff --git i/tests/vircgroupmock.c w/tests/vircgroupmock.c
index 06bd0a5f29..83f43e72bf 100644
--- i/tests/vircgroupmock.c
+++ w/tests/vircgroupmock.c
@@ -459,9 +459,7 @@ FILE *fopen(const char *path, const char *mode)
          }
      }

-    if (type) {
-        if (!filename)
-            abort();
+    if (filename && type) {
          if (virAsprintfQuiet(&filepath, "%s/vircgroupdata/%s.%s",
                               abs_srcdir, filename, type) < 0) {
              abort();



Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tests: don't abort in fopen(/proc/mounts)
Posted by Daniel P. Berrangé 5 years ago 
On Wed, Mar 27, 2019 at 12:55:18PM +0100, Michal Privoznik wrote:
> On 3/27/19 11:31 AM, Daniel P. Berrangé wrote:
> > The mock fopen() function will abort if "/proc/mounts" is
> > requested with "r" permissions and VIR_CGROUP_MOCK_FILENAME
> > env var is not set.
> > 
> > Unfortunately this is triggering by the libselinux library
> > constructor when it tries to read /proc/mounts to find out
> > if selinuxfs is mounted in an unusual place.
> > 
> > This, however, only affects libselinux in Debian as that
> > opens with "r", while in Fedora / RHEL it opens "re" and
> > thus luckily never triggered the abort(), instead getting
> > an EACCESS.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >   tests/vircgroupmock.c | 6 ++++--
> >   1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tests/vircgroupmock.c b/tests/vircgroupmock.c
> > index 06bd0a5f29..9c67a44b0d 100644
> > --- a/tests/vircgroupmock.c
> > +++ b/tests/vircgroupmock.c
> > @@ -460,8 +460,10 @@ FILE *fopen(const char *path, const char *mode)
> >       }
> >       if (type) {
> > -        if (!filename)
> > -            abort();
> > +        if (!filename) {
> > +            errno = EACCES;
> > +            return NULL;
> > +        }
> >           if (virAsprintfQuiet(&filepath, "%s/vircgroupdata/%s.%s",
> >                                abs_srcdir, filename, type) < 0) {
> >               abort();
> > 
> 
> How about:
> 
> diff --git i/tests/vircgroupmock.c w/tests/vircgroupmock.c
> index 06bd0a5f29..83f43e72bf 100644
> --- i/tests/vircgroupmock.c
> +++ w/tests/vircgroupmock.c
> @@ -459,9 +459,7 @@ FILE *fopen(const char *path, const char *mode)
>          }
>      }
> 
> -    if (type) {
> -        if (!filename)
> -            abort();
> +    if (filename && type) {
>          if (virAsprintfQuiet(&filepath, "%s/vircgroupdata/%s.%s",
>                               abs_srcdir, filename, type) < 0) {
>              abort();

That will cause it to try to open the real /proc/mounts, which feels
like an undesirable thing as it means the test could silently start
relying on host state by mistake.

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tests: don't abort in fopen(/proc/mounts)
Posted by Michal Privoznik 5 years ago 
On 3/27/19 12:57 PM, Daniel P. Berrangé wrote:
> On Wed, Mar 27, 2019 at 12:55:18PM +0100, Michal Privoznik wrote:
>> On 3/27/19 11:31 AM, Daniel P. Berrangé wrote:
>>> The mock fopen() function will abort if "/proc/mounts" is
>>> requested with "r" permissions and VIR_CGROUP_MOCK_FILENAME
>>> env var is not set.
>>>
>>> Unfortunately this is triggering by the libselinux library
>>> constructor when it tries to read /proc/mounts to find out
>>> if selinuxfs is mounted in an unusual place.
>>>
>>> This, however, only affects libselinux in Debian as that
>>> opens with "r", while in Fedora / RHEL it opens "re" and
>>> thus luckily never triggered the abort(), instead getting
>>> an EACCESS.
>>>
>>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>>> ---
>>>    tests/vircgroupmock.c | 6 ++++--
>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tests/vircgroupmock.c b/tests/vircgroupmock.c
>>> index 06bd0a5f29..9c67a44b0d 100644
>>> --- a/tests/vircgroupmock.c
>>> +++ b/tests/vircgroupmock.c
>>> @@ -460,8 +460,10 @@ FILE *fopen(const char *path, const char *mode)
>>>        }
>>>        if (type) {
>>> -        if (!filename)
>>> -            abort();
>>> +        if (!filename) {
>>> +            errno = EACCES;
>>> +            return NULL;
>>> +        }
>>>            if (virAsprintfQuiet(&filepath, "%s/vircgroupdata/%s.%s",
>>>                                 abs_srcdir, filename, type) < 0) {
>>>                abort();
>>>
>>
>> How about:
>>
>> diff --git i/tests/vircgroupmock.c w/tests/vircgroupmock.c
>> index 06bd0a5f29..83f43e72bf 100644
>> --- i/tests/vircgroupmock.c
>> +++ w/tests/vircgroupmock.c
>> @@ -459,9 +459,7 @@ FILE *fopen(const char *path, const char *mode)
>>           }
>>       }
>>
>> -    if (type) {
>> -        if (!filename)
>> -            abort();
>> +    if (filename && type) {
>>           if (virAsprintfQuiet(&filepath, "%s/vircgroupdata/%s.%s",
>>                                abs_srcdir, filename, type) < 0) {
>>               abort();
> 
> That will cause it to try to open the real /proc/mounts, which feels
> like an undesirable thing as it means the test could silently start
> relying on host state by mistake.
> 

Ah, good point. ACK to your version then.

Michal

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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.