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

[Laszlo Ersek]

On 4/26/23 16:47, Richard W.M. Jones wrote:
> 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:

Ah, thanks for checking that.

That brings back some (distant) memories.

I've repeatedly used getopt() myself in the past, it's just that I could
never see the point of letting getopt() print an error message on its
own. Also I always wanted invalid options to be reported differently to
me from missing option arguments. For those reasons, I'd always place a
<colon> at the front of optstring. I'd gotten so used to that practice
that now it simply didn't occur to me that any application might rely on
the diagnostic message printed by getopt().

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

We always depend on the setfiles utility, but with the above
information, we also depend on the host's libc -- after all, the
appliance gets built from the host's packages. From
"appliance/packagelist.in", we try to support REDHAT, DEBIAN, UBUNTU,
ARCHLINUX, SUSE, FRUGALWARE, MAGEIA; do they all default to glibc? (I've
only heard of Alma Linux using musl, but who knows...)

On the other hand... I realize that even *pre-patch*; we already depend
on getopt()'s *internally-printed* "'X'" substring! In other words, this
patch does not make our dependency on libc internals stronger than it
already is.

So, my R-b stands.

Thanks!
Laszlo

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