util: Don't spawn pkttyagent when stdin is not a tty

[PATCH] util: Don't spawn pkttyagent when stdin is not a tty

Posted by Martin Kletzander 2 years, 4 months ago

My idea was that running pkttyagent unconditionally, modulo checks that
pkttyagent itself does to make sure it does not fail, is not going to be an
issue turned out to be wrong.  Adding back the original check for stdin being a
tty helps in some testing scenarios as reported by Jim Fehlig and does not
really cause any issues.  I originally wanted it in because it also made
pkttyagent auth work with redirected input into virsh (with a connection that
requires polkit authentication and without a session-wide polkit tty agent,
basically making pkttyagent necessary to succeed).  But anyone running virsh
like that is asking for problems already anyway =)

Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
---
 src/util/virpolkit.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c
index 8970de192fe1..bb68b5a59614 100644
--- a/src/util/virpolkit.c
+++ b/src/util/virpolkit.c
@@ -237,6 +237,9 @@ virPolkitAgentAvailable(void)
     const char *termid = ctermid(NULL);
     VIR_AUTOCLOSE fd = -1;
 
+    if (!isatty(STDIN_FILENO))
+        return false;
+
     if (!virFileIsExecutable(PKTTYAGENT))
         return false;
 
-- 
2.34.1

Re: [PATCH] util: Don't spawn pkttyagent when stdin is not a tty

Posted by Martin Kletzander 2 years, 4 months ago

On Sat, Dec 11, 2021 at 02:23:11PM +0100, Martin Kletzander wrote:
>My idea was that running pkttyagent unconditionally, modulo checks that
>pkttyagent itself does to make sure it does not fail, is not going to be an
>issue turned out to be wrong.  Adding back the original check for stdin being a
>tty helps in some testing scenarios as reported by Jim Fehlig and does not
>really cause any issues.  I originally wanted it in because it also made
>pkttyagent auth work with redirected input into virsh (with a connection that
>requires polkit authentication and without a session-wide polkit tty agent,
>basically making pkttyagent necessary to succeed).  But anyone running virsh
>like that is asking for problems already anyway =)
>
>Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>---

Of course I added a note that this would be a patch that can go in if we
figure out the real root cause, then sent it to a wrong address, then
formatted it again properly, sent it to the right address ad forgot to
add the note again 🤦

> src/util/virpolkit.c | 3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c
>index 8970de192fe1..bb68b5a59614 100644
>--- a/src/util/virpolkit.c
>+++ b/src/util/virpolkit.c
>@@ -237,6 +237,9 @@ virPolkitAgentAvailable(void)
>     const char *termid = ctermid(NULL);
>     VIR_AUTOCLOSE fd = -1;
>
>+    if (!isatty(STDIN_FILENO))
>+        return false;
>+
>     if (!virFileIsExecutable(PKTTYAGENT))
>         return false;
>
>-- 
>2.34.1
>

Re: [PATCH] util: Don't spawn pkttyagent when stdin is not a tty

Posted by Michal Prívozník 2 years, 4 months ago

On 12/11/21 14:23, Martin Kletzander wrote:
> My idea was that running pkttyagent unconditionally, modulo checks that
> pkttyagent itself does to make sure it does not fail, is not going to be an
> issue turned out to be wrong.  Adding back the original check for stdin being a
> tty helps in some testing scenarios as reported by Jim Fehlig and does not
> really cause any issues.  I originally wanted it in because it also made
> pkttyagent auth work with redirected input into virsh (with a connection that
> requires polkit authentication and without a session-wide polkit tty agent,
> basically making pkttyagent necessary to succeed).  But anyone running virsh
> like that is asking for problems already anyway =)
> 
> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> ---
>  src/util/virpolkit.c | 3 +++
>  1 file changed, 3 insertions(+)

In case you want to push this:

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

But I'd really like to get to the bottom of this. Why does sleep()
suspends for that long and why isn't polkit-agent killed?

Michal

Re: [PATCH] util: Don't spawn pkttyagent when stdin is not a tty

Posted by Jim Fehlig 2 years, 4 months ago

On 12/17/21 7:01 AM, Michal Prívozník wrote:
> On 12/11/21 14:23, Martin Kletzander wrote:
>> My idea was that running pkttyagent unconditionally, modulo checks that
>> pkttyagent itself does to make sure it does not fail, is not going to be an
>> issue turned out to be wrong.  Adding back the original check for stdin being a
>> tty helps in some testing scenarios as reported by Jim Fehlig and does not
>> really cause any issues.  I originally wanted it in because it also made
>> pkttyagent auth work with redirected input into virsh (with a connection that
>> requires polkit authentication and without a session-wide polkit tty agent,
>> basically making pkttyagent necessary to succeed).  But anyone running virsh
>> like that is asking for problems already anyway =)
>>
>> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>> ---
>>   src/util/virpolkit.c | 3 +++
>>   1 file changed, 3 insertions(+)
> 
> In case you want to push this:
> 
> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

And

Tested-by: Jim Fehlig <jfehlig@suse.com>

> 
> But I'd really like to get to the bottom of this. Why does sleep()
> suspends for that long and why isn't polkit-agent killed?

I got sidetracked before I could dig deeper, however I did notice the test 
process and all children were in a stopped state per /proc/<pid>/status. I sent 
SIGCONT to the processes and the test successfully completed. I'm baffled why 
these processes become stopped when pkttyagent is in the picture and work fine 
otherwise.

Jim

Re: [PATCH] util: Don't spawn pkttyagent when stdin is not a tty

Posted by Jim Fehlig 2 years, 4 months ago

On 12/17/21 9:25 AM, Jim Fehlig wrote:
> On 12/17/21 7:01 AM, Michal Prívozník wrote:
>> On 12/11/21 14:23, Martin Kletzander wrote:
>>> My idea was that running pkttyagent unconditionally, modulo checks that
>>> pkttyagent itself does to make sure it does not fail, is not going to be an
>>> issue turned out to be wrong.  Adding back the original check for stdin being a
>>> tty helps in some testing scenarios as reported by Jim Fehlig and does not
>>> really cause any issues.  I originally wanted it in because it also made
>>> pkttyagent auth work with redirected input into virsh (with a connection that
>>> requires polkit authentication and without a session-wide polkit tty agent,
>>> basically making pkttyagent necessary to succeed).  But anyone running virsh
>>> like that is asking for problems already anyway =)
>>>
>>> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>>> ---
>>>   src/util/virpolkit.c | 3 +++
>>>   1 file changed, 3 insertions(+)
>>
>> In case you want to push this:
>>
>> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
> 
> And
> 
> Tested-by: Jim Fehlig <jfehlig@suse.com>
> 
>>
>> But I'd really like to get to the bottom of this. Why does sleep()
>> suspends for that long and why isn't polkit-agent killed?
> 
> I got sidetracked before I could dig deeper, however I did notice the test 
> process and all children were in a stopped state per /proc/<pid>/status. I sent 
> SIGCONT to the processes and the test successfully completed. I'm baffled why 
> these processes become stopped when pkttyagent is in the picture and work fine 
> otherwise.

I forgot to mention, it's likely a problem in the test framework code, which is 
apparently unmaintained by still used by SUSE QA. "I got sidetracked" is a nice 
way to say I didn't have a lot of motivation to read through a gob of old, 
unfamiliar perl code :-).

Jim