diff mbox series

support: Close original descriptors in support_capture_subprocess

Message ID 87muppo5hp.fsf@oldenburg.str.redhat.com
State New
Headers show
Series support: Close original descriptors in support_capture_subprocess | expand

Commit Message

Florian Weimer Dec. 1, 2018, 11:23 a.m. UTC 
2018-12-01  Florian Weimer  <fweimer@redhat.com>

	* support/support_capture_subprocess.c
	(support_capture_subprocess): Close original pipe descriptors in
	subprocess.

Comments

Andreas Schwab Dec. 1, 2018, 2:50 p.m. UTC | #1 
On Dez 01 2018, Florian Weimer <fweimer@redhat.com> wrote:

> diff --git a/support/support_capture_subprocess.c b/support/support_capture_subprocess.c
> index 6d2029e13b..e1efcd52b0 100644
> --- a/support/support_capture_subprocess.c
> +++ b/support/support_capture_subprocess.c
> @@ -72,6 +72,10 @@ support_capture_subprocess (void (*callback) (void *), void *closure)
>        xclose (stderr_pipe[0]);
>        xdup2 (stdout_pipe[1], STDOUT_FILENO);
>        xdup2 (stderr_pipe[1], STDERR_FILENO);
> +      if (stdout_pipe[1] > STDERR_FILENO)
> +        xclose (stdout_pipe[1]);
> +      if (stderr_pipe[1] > STDERR_FILENO)
> +        xclose (stderr_pipe[1]);

Is there any reason pipe would have handed out 0, 1 or 2?

Andreas.
Florian Weimer Dec. 1, 2018, 2:53 p.m. UTC | #2 
* Andreas Schwab:

> On Dez 01 2018, Florian Weimer <fweimer@redhat.com> wrote:
>
>> diff --git a/support/support_capture_subprocess.c b/support/support_capture_subprocess.c
>> index 6d2029e13b..e1efcd52b0 100644
>> --- a/support/support_capture_subprocess.c
>> +++ b/support/support_capture_subprocess.c
>> @@ -72,6 +72,10 @@ support_capture_subprocess (void (*callback) (void *), void *closure)
>>        xclose (stderr_pipe[0]);
>>        xdup2 (stdout_pipe[1], STDOUT_FILENO);
>>        xdup2 (stderr_pipe[1], STDERR_FILENO);
>> +      if (stdout_pipe[1] > STDERR_FILENO)
>> +        xclose (stdout_pipe[1]);
>> +      if (stderr_pipe[1] > STDERR_FILENO)
>> +        xclose (stderr_pipe[1]);
>
> Is there any reason pipe would have handed out 0, 1 or 2?

It really should not have happened unless the caller (the test case) has
closed the 0/1/2 descriptors.  That should not happen, but maybe there
will be a test some day where this is needed.

Thanks,
Florian
Andreas Schwab Dec. 1, 2018, 8 p.m. UTC | #3 
On Dez 01 2018, Florian Weimer <fweimer@redhat.com> wrote:

> * Andreas Schwab:
>
>> On Dez 01 2018, Florian Weimer <fweimer@redhat.com> wrote:
>>
>>> diff --git a/support/support_capture_subprocess.c b/support/support_capture_subprocess.c
>>> index 6d2029e13b..e1efcd52b0 100644
>>> --- a/support/support_capture_subprocess.c
>>> +++ b/support/support_capture_subprocess.c
>>> @@ -72,6 +72,10 @@ support_capture_subprocess (void (*callback) (void *), void *closure)
>>>        xclose (stderr_pipe[0]);
>>>        xdup2 (stdout_pipe[1], STDOUT_FILENO);
>>>        xdup2 (stderr_pipe[1], STDERR_FILENO);
>>> +      if (stdout_pipe[1] > STDERR_FILENO)
>>> +        xclose (stdout_pipe[1]);
>>> +      if (stderr_pipe[1] > STDERR_FILENO)
>>> +        xclose (stderr_pipe[1]);
>>
>> Is there any reason pipe would have handed out 0, 1 or 2?
>
> It really should not have happened unless the caller (the test case) has
> closed the 0/1/2 descriptors.  That should not happen, but maybe there
> will be a test some day where this is needed.

If one of them is 0 you still want to close it, don't you?

Andreas.
Florian Weimer Dec. 1, 2018, 8:07 p.m. UTC | #4 
* Andreas Schwab:

> On Dez 01 2018, Florian Weimer <fweimer@redhat.com> wrote:
>
>> * Andreas Schwab:
>>
>>> On Dez 01 2018, Florian Weimer <fweimer@redhat.com> wrote:
>>>
>>>> diff --git a/support/support_capture_subprocess.c b/support/support_capture_subprocess.c
>>>> index 6d2029e13b..e1efcd52b0 100644
>>>> --- a/support/support_capture_subprocess.c
>>>> +++ b/support/support_capture_subprocess.c
>>>> @@ -72,6 +72,10 @@ support_capture_subprocess (void (*callback) (void *), void *closure)
>>>>        xclose (stderr_pipe[0]);
>>>>        xdup2 (stdout_pipe[1], STDOUT_FILENO);
>>>>        xdup2 (stderr_pipe[1], STDERR_FILENO);
>>>> +      if (stdout_pipe[1] > STDERR_FILENO)
>>>> +        xclose (stdout_pipe[1]);
>>>> +      if (stderr_pipe[1] > STDERR_FILENO)
>>>> +        xclose (stderr_pipe[1]);
>>>
>>> Is there any reason pipe would have handed out 0, 1 or 2?
>>
>> It really should not have happened unless the caller (the test case) has
>> closed the 0/1/2 descriptors.  That should not happen, but maybe there
>> will be a test some day where this is needed.
>
> If one of them is 0 you still want to close it, don't you?

Hmm.  We create four descriptors:

  int stdout_pipe[2];
  xpipe (stdout_pipe);
  int stderr_pipe[2];
  xpipe (stderr_pipe);

This means that stdout_pipe[1] is at least 1 and stderr_pipe[1] is at
least 3.  We can never close descriptor 0, but the first xclose could
close descriptor 1 or 2.  Should I remove the second if condition?

Thanks,
Florian
Andreas Schwab Dec. 1, 2018, 8:20 p.m. UTC | #5 
On Dez 01 2018, Florian Weimer <fweimer@redhat.com> wrote:

> This means that stdout_pipe[1] is at least 1 and stderr_pipe[1] is at
> least 3.  We can never close descriptor 0, but the first xclose could
> close descriptor 1 or 2.  Should I remove the second if condition?

IMHO it would be cleaner that if a test needs a closed standard fd there
should be a separate functionality to set that up instead of letting the
test close it directly.

Andreas.
Florian Weimer Dec. 1, 2018, 9:04 p.m. UTC | #6 
* Andreas Schwab:

> On Dez 01 2018, Florian Weimer <fweimer@redhat.com> wrote:
>
>> This means that stdout_pipe[1] is at least 1 and stderr_pipe[1] is at
>> least 3.  We can never close descriptor 0, but the first xclose could
>> close descriptor 1 or 2.  Should I remove the second if condition?
>
> IMHO it would be cleaner that if a test needs a closed standard fd there
> should be a separate functionality to set that up instead of letting the
> test close it directly.

What about this?

Thanks,
Florian

2018-12-01  Florian Weimer  <fweimer@redhat.com>

	* support/support_capture_subprocess.c
	(support_capture_subprocess): Check that pipe descriptors have
	expected values.  Close original pipe descriptors in subprocess.

diff --git a/support/support_capture_subprocess.c b/support/support_capture_subprocess.c
index 6d2029e13b..93f6ea3102 100644
--- a/support/support_capture_subprocess.c
+++ b/support/support_capture_subprocess.c
@@ -59,8 +59,12 @@ support_capture_subprocess (void (*callback) (void *), void *closure)
 
   int stdout_pipe[2];
   xpipe (stdout_pipe);
+  TEST_VERIFY (stdout_pipe[0] > STDERR_FILENO);
+  TEST_VERIFY (stdout_pipe[1] > STDERR_FILENO);
   int stderr_pipe[2];
   xpipe (stderr_pipe);
+  TEST_VERIFY (stderr_pipe[0] > STDERR_FILENO);
+  TEST_VERIFY (stderr_pipe[1] > STDERR_FILENO);
 
   TEST_VERIFY (fflush (stdout) == 0);
   TEST_VERIFY (fflush (stderr) == 0);
@@ -72,6 +76,8 @@ support_capture_subprocess (void (*callback) (void *), void *closure)
       xclose (stderr_pipe[0]);
       xdup2 (stdout_pipe[1], STDOUT_FILENO);
       xdup2 (stderr_pipe[1], STDERR_FILENO);
+      xclose (stdout_pipe[1]);
+      xclose (stderr_pipe[1]);
       callback (closure);
       _exit (0);
     }
Andreas Schwab Dec. 1, 2018, 9:14 p.m. UTC | #7 
On Dez 01 2018, Florian Weimer <fweimer@redhat.com> wrote:

> 2018-12-01  Florian Weimer  <fweimer@redhat.com>
>
> 	* support/support_capture_subprocess.c
> 	(support_capture_subprocess): Check that pipe descriptors have
> 	expected values.  Close original pipe descriptors in subprocess.

Ok.

Andreas.
diff mbox series

Patch

diff --git a/support/support_capture_subprocess.c b/support/support_capture_subprocess.c
index 6d2029e13b..e1efcd52b0 100644
--- a/support/support_capture_subprocess.c
+++ b/support/support_capture_subprocess.c
@@ -72,6 +72,10 @@  support_capture_subprocess (void (*callback) (void *), void *closure)
       xclose (stderr_pipe[0]);
       xdup2 (stdout_pipe[1], STDOUT_FILENO);
       xdup2 (stderr_pipe[1], STDERR_FILENO);
+      if (stdout_pipe[1] > STDERR_FILENO)
+        xclose (stdout_pipe[1]);
+      if (stderr_pipe[1] > STDERR_FILENO)
+        xclose (stderr_pipe[1]);
       callback (closure);
       _exit (0);
     }