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

Commit Message

Florian Weimer Dec. 1, 2018, 11:23 a.m.
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. | #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.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."
Florian Weimer Dec. 1, 2018, 2:53 p.m. | #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. | #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.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."
Florian Weimer Dec. 1, 2018, 8:07 p.m. | #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. | #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.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."
Florian Weimer Dec. 1, 2018, 9:04 p.m. | #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. | #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.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

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);
     }