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

[Michal Privoznik]

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