Libguestfs - Apr 2023 - [PATCH 2/3] daemon/selinux-relabel: search for "invalid option" in setfiles output

On 4/26/23 14:59, Andrey Drobyshev wrote:
> 'X' in the setiles' stderr doesn't necessarily mean that
option 'X'
> doesn't exist.  For instance, when passing '-T' we get:
"setfiles:
> option requires an argument -- 'T'".
> 
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev at virtuozzo.com>
> ---
>  daemon/selinux-relabel.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/daemon/selinux-relabel.c b/daemon/selinux-relabel.c
> index 454486c17..60a6f48a9 100644
> --- a/daemon/selinux-relabel.c
> +++ b/daemon/selinux-relabel.c
> @@ -56,8 +56,9 @@ setfiles_has_option (int *flag, char opt_char)
>  
>    if (*flag == -1) {
>      char option[] = { '-', opt_char, '\0' };       /*
"-X" */
> -    char err_opt[] = { '\'', opt_char, '\'',
'\0'}; /* "'X'" */
> +    char err_opt[32];     /* "invalid option -- 'X'" */
>  
> +    snprintf(err_opt, sizeof(err_opt), "invalid option --
'%c'", opt_char);
>      ignore_value (command (NULL, &err, "setfiles", option,
NULL));
>      *flag = err && strstr (err, /* "invalid option -- "
*/ err_opt) == NULL;
>    }
Can you check in the selinux library git history how far back the

  invalid option -- '%c'

message can be relied upon?


Other than that, I'd suggest a number of superficial updates, but for a
change, I won't obsess about them.

series
Reviewed-by: Laszlo Ersek <lersek at redhat.com>

(We shouldn't merge this until Rich agrees, too.)

Laszlo

On Wed, Apr 26, 2023 at 04:37:21PM +0200, Laszlo Ersek
wrote:
> On 4/26/23 14:59, Andrey Drobyshev wrote:
> > 'X' in the setiles' stderr doesn't necessarily mean
that option 'X'
> > doesn't exist.  For instance, when passing '-T' we get:
"setfiles:
> > option requires an argument -- 'T'".
> > 
> > Signed-off-by: Andrey Drobyshev <andrey.drobyshev at
virtuozzo.com>
> > ---
> >  daemon/selinux-relabel.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/daemon/selinux-relabel.c b/daemon/selinux-relabel.c
> > index 454486c17..60a6f48a9 100644
> > --- a/daemon/selinux-relabel.c
> > +++ b/daemon/selinux-relabel.c
> > @@ -56,8 +56,9 @@ setfiles_has_option (int *flag, char opt_char)
> >  
> >    if (*flag == -1) {
> >      char option[] = { '-', opt_char, '\0' };       /*
"-X" */
> > -    char err_opt[] = { '\'', opt_char, '\'',
'\0'}; /* "'X'" */
> > +    char err_opt[32];     /* "invalid option --
'X'" */
> >  
> > +    snprintf(err_opt, sizeof(err_opt), "invalid option --
'%c'", opt_char);
> >      ignore_value (command (NULL, &err, "setfiles",
option, NULL));
> >      *flag = err && strstr (err, /* "invalid option --
" */ err_opt) == NULL;
> >    }
> 
> Can you check in the selinux library git history how far back the
> 
>   invalid option -- '%c'
> 
> message can be relied upon?
It actually comes from glibc:

https://sourceware.org/git/?p=glibc.git;a=blob;f=posix/getopt.c;h=1e2441c4afee5d005b430b6de875a4c7d05dcb28;hb=HEAD#l621

It's actually been around "forever" ..  It exists in the initial
import (to CVS?) from 1995:

https://sourceware.org/git/?p=glibc.git;a=blob;f=posix/posix/getopt.c;h=7e7fdc7c3b476d8295c3776b20ed0dd06055dcf3;hb=HEAD#l613

and there's even a claim there about POSIX -- but it would only apply
for POSIXLY_CORRECT which we don't use in the daemon.

But yes it looks safe!

Rich.
> 
> Other than that, I'd suggest a number of superficial updates, but for a
> change, I won't obsess about them.
> 
> series
> Reviewed-by: Laszlo Ersek <lersek at redhat.com>
> 
> (We shouldn't merge this until Rich agrees, too.)
> 
> Laszlo
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit