Message ID | 87muppo5hp.fsf@oldenburg.str.redhat.com |
---|---|
State | Superseded |
Headers |
Received: (qmail 47005 invoked by alias); 1 Dec 2018 11:23:43 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 46995 invoked by uid 89); 1 Dec 2018 11:23:42 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_HELO_PASS, TVD_SPACE_RATIO autolearn=ham version=3.3.2 spammy=closure, Hx-languages-length:807 X-HELO: mx1.redhat.com From: Florian Weimer <fweimer@redhat.com> To: libc-alpha@sourceware.org Subject: [PATCH] support: Close original descriptors in support_capture_subprocess Date: Sat, 01 Dec 2018 12:23:30 +0100 Message-ID: <87muppo5hp.fsf@oldenburg.str.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain |
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
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: > 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
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: > 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
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.
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); }