[libvirt] [PATCH] util: Don't overflow on errno in virFileAccessibleAs

[Michal Privoznik]

On 08.03.2012 14:08, Eric Blake wrote:
> On 03/08/2012 03:37 AM, Michal Privoznik wrote:
>> If we need to virFork() to check assess() under different
>> UID+GID we need to translate returned status via WEXITSTATUS().
>> Otherwise, we may return values greater than 255 which is
>> obviously wrong.
>> ---
>>  src/util/util.c |    7 ++++++-
>>  1 files changed, 6 insertions(+), 1 deletions(-)
>>
>> diff --git a/src/util/util.c b/src/util/util.c
>> index 548ed1c..15e6cfa 100644
>> --- a/src/util/util.c
>> +++ b/src/util/util.c
>> @@ -724,8 +724,13 @@ virFileAccessibleAs(const char *path, int mode,
>>                  return -1;
>>          }
>>  
>> +        if (!WIFEXITED(status)) {
>> +            errno = EINTR;
>> +            return -1;
>> +        }
> 
> ACK; this matches what we do in virFileOpenForked.

Thanks pushed.
> 
> However, I still see two lingering issues that might be worth revisiting:
> 
> 1. I wonder if virWaitPid() would be easier to use if it only returned
> success on WIFEXITED, and set *status to WEXITSTAUS(), while returning
> -1 on any child dying due to a signal.  I'd have to audit the users of
> virWaitPid to see if they can all be simplified by this change, or if
> there really is a user that needs to know if a child exited due to a signal.

yes, i was wondering about this too when writing the patch. However I
took the quicker way. Let me see if i can produce cleanup patch as
you've described it.
> 
> 2. This still shares the latent bug in virFileOpenForked that errno is
> not always guaranteed to be less than 255; on GNU Hurd, this code is
> broken - but libvirt doesn't compile on Hurd.  A true fix would be to
> enumerate specific errno values to specific exit codes, and map all
> others to a catch-all; see how daemon/libvirtd.c has virDaemonErr for
> this purpose.
> 

Yeah, since we don't compile on Hurd anyway, I wouldn't take much care
here. I am not saying we should make it intentionally harder for a
developer trying to make libvirt compile there, but why unnecessarily
bound ourselves?

Michal