Re: [PATCH trivial v3] qga: use access(2) to check for command existance instead of questionable stat(2)

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 1/11/25 14:14, Michael Tokarev wrote:
> The code checks existance of a command (halt/poweroff/reboot) by using
> stat(2) and immediately checking for S_ISLNK() on the returned stat
> struct.  This check will never be true, because stat(2) always follows
> symbolic links and hence will either return ENOENT (in case of dangling
> symlink) or the properties for the final target file.  It is lstat(2)
> which might return information about the symlink itself.  However, even
> there, we want to check the final file properties, not the first symlink.
> 
> This check - S_ISLNK - is harmful but useless in this case.  However, it
> is confusing and it helps the wrong usage of stat(2) to spread, so it is
> better to remove it.
> 
> Additionally, the code would better to check for the executable bits
> of the final file, not check if it's a regular file - it's sort of
> dubious to have anything but regular files in /sbin/.
> 
> But a POSIX system provides another command which suits the purpose
> perfectly: it is access(2).  And it is so simple that it's not
> necessary to create a separate function when usin it.
> 
> Replace stat(2) with access(X_OK) to check for file existance in
> qga/commands-posix.c
> 
> Fixes: c5b4afd4d56e "qga: Support guest shutdown of BusyBox-based systems"
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
> v3: and actually don't forget to commit the changes.
>      I'm sorry for the noize.
> v2: fix reverse logic of the access() tests.
>      I should write code more often :)
> 
>   qga/commands-posix.c | 12 +++---------
>   1 file changed, 3 insertions(+), 9 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>