From e365a3198da2dba8a0b174f257d7eac9256389ba Mon Sep 17 00:00:00 2001 From: Nightt <87569709+nightt5879@users.noreply.github.com> Date: Fri, 29 May 2026 15:27:17 +0800 Subject: [PATCH 1/2] system/popen: Avoid copying FILE Track popen streams in a small private list so pclose() can find the spawned shell pid without embedding a copied FILE object in the tracking container. This keeps the returned stream owned by fdopen()/fclose() and avoids depending on libc FILE layout details. Fixes #2937. Signed-off-by: Nightt <87569709+nightt5879@users.noreply.github.com> --- system/popen/popen.c | 109 ++++++++++++++++++++++++++++++++----------- 1 file changed, 83 insertions(+), 26 deletions(-) diff --git a/system/popen/popen.c b/system/popen/popen.c index b52fbaafe85..f338bf696a0 100644 --- a/system/popen/popen.c +++ b/system/popen/popen.c @@ -32,6 +32,7 @@ #include #include #include +#include #include #include #include @@ -45,17 +46,78 @@ * Private Types ****************************************************************************/ -/* struct popen_file_s is a cast compatible version of FILE that contains - * the additional PID of the shell processes needed by pclose(). - */ - struct popen_file_s { - FILE copy; - FILE *original; + FAR struct popen_file_s *flink; + FAR FILE *stream; pid_t shell; }; +/**************************************************************************** + * Private Data + ****************************************************************************/ + +static sem_t g_popen_sem = SEM_INITIALIZER(1); +static FAR struct popen_file_s *g_popen_files; + +/**************************************************************************** + * Private Functions + ****************************************************************************/ + +static void popen_lock(void) +{ + while (sem_wait(&g_popen_sem) < 0) + { + int errcode = errno; + + DEBUGASSERT(errcode == EINTR || errcode == ECANCELED); + UNUSED(errcode); + } +} + +static void popen_unlock(void) +{ + sem_post(&g_popen_sem); +} + +static void popen_add_file(FAR struct popen_file_s *container) +{ + popen_lock(); + container->flink = g_popen_files; + g_popen_files = container; + popen_unlock(); +} + +static FAR struct popen_file_s *popen_remove_file(FAR FILE *stream) +{ + FAR struct popen_file_s *container; + FAR struct popen_file_s *prev = NULL; + + popen_lock(); + for (container = g_popen_files; container != NULL; + container = container->flink) + { + if (container->stream == stream) + { + if (prev == NULL) + { + g_popen_files = container->flink; + } + else + { + prev->flink = container->flink; + } + + break; + } + + prev = container; + } + + popen_unlock(); + return container; +} + /**************************************************************************** * Public Functions ****************************************************************************/ @@ -193,8 +255,8 @@ FILE *popen(FAR const char *command, FAR const char *mode) /* Create the FILE stream return reference */ - container->original = fdopen(retfd, mode); - if (container->original == NULL) + container->stream = fdopen(retfd, mode); + if (container->stream == NULL) { errcode = errno; goto errout_with_pipe; @@ -329,10 +391,10 @@ FILE *popen(FAR const char *command, FAR const char *mode) ioctl(retfd, FIOCLEX, 0); } - /* Finale and return input input/output stream */ + /* Save the shell PID for pclose() without depending on FILE internals. */ - memcpy(&container->copy, container->original, sizeof(FILE)); - return &container->copy; + popen_add_file(container); + return container->stream; errout_with_actions: posix_spawn_file_actions_destroy(&file_actions); @@ -341,7 +403,7 @@ FILE *popen(FAR const char *command, FAR const char *mode) posix_spawnattr_destroy(&attr); errout_with_stream: - fclose(container->original); + fclose(container->stream); errout_with_pipe: close(fd[0]); @@ -399,28 +461,23 @@ FILE *popen(FAR const char *command, FAR const char *mode) int pclose(FILE *stream) { - FAR struct popen_file_s *container = (FAR struct popen_file_s *)stream; - FILE *original; + FAR struct popen_file_s *container; pid_t shell; #ifdef CONFIG_SCHED_WAITPID int status; int result; #endif - DEBUGASSERT(container != NULL && container->original != NULL); - original = container->original; - - /* Set the state of the original file descriptor to the state of the - * working copy - */ - - memcpy(original, &container->copy, sizeof(FILE)); + DEBUGASSERT(stream != NULL); - /* Then close the original and free the container (saving the PID of the - * shell process) - */ + container = popen_remove_file(stream); + if (container == NULL) + { + errno = EINVAL; + return ERROR; + } - fclose(original); + fclose(stream); shell = container->shell; free(container); From 23a698fde9de2f8b41c3e6868a415f5a13eed6d1 Mon Sep 17 00:00:00 2001 From: Nightt <87569709+nightt5879@users.noreply.github.com> Date: Fri, 29 May 2026 21:36:26 +0800 Subject: [PATCH 2/2] testing/libc: Add popen test Add a small popen test that opens two read streams, verifies their output, and closes the first stream before the second. This covers reading from the returned popen stream and exercises pclose lookup/removal when the stream is not the first entry in the tracking list. Signed-off-by: Nightt <87569709+nightt5879@users.noreply.github.com> --- testing/libc/popen/CMakeLists.txt | 33 +++++++ testing/libc/popen/Kconfig | 12 +++ testing/libc/popen/Make.defs | 25 +++++ testing/libc/popen/Makefile | 34 +++++++ testing/libc/popen/popen_test.c | 146 ++++++++++++++++++++++++++++++ 5 files changed, 250 insertions(+) create mode 100644 testing/libc/popen/CMakeLists.txt create mode 100644 testing/libc/popen/Kconfig create mode 100644 testing/libc/popen/Make.defs create mode 100644 testing/libc/popen/Makefile create mode 100644 testing/libc/popen/popen_test.c diff --git a/testing/libc/popen/CMakeLists.txt b/testing/libc/popen/CMakeLists.txt new file mode 100644 index 00000000000..2cd8d005111 --- /dev/null +++ b/testing/libc/popen/CMakeLists.txt @@ -0,0 +1,33 @@ +# ############################################################################## +# apps/testing/libc/popen/CMakeLists.txt +# +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed to the Apache Software Foundation (ASF) under one or more contributor +# license agreements. See the NOTICE file distributed with this work for +# additional information regarding copyright ownership. The ASF licenses this +# file to you under the Apache License, Version 2.0 (the "License"); you may not +# use this file except in compliance with the License. You may obtain a copy of +# the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations under +# the License. +# +# ############################################################################## + +if(CONFIG_TESTING_POPEN_TEST) + nuttx_add_application( + NAME + popen_test + STACKSIZE + 4096 + MODULE + ${CONFIG_TESTING_POPEN_TEST} + SRCS + popen_test.c) +endif() diff --git a/testing/libc/popen/Kconfig b/testing/libc/popen/Kconfig new file mode 100644 index 00000000000..b2b1bd76068 --- /dev/null +++ b/testing/libc/popen/Kconfig @@ -0,0 +1,12 @@ +# +# For a description of the syntax of this configuration file, +# see the file kconfig-language.txt in the NuttX tools repository. +# + +config TESTING_POPEN_TEST + tristate "popen() test" + default n + depends on SYSTEM_POPEN + depends on !NSH_DISABLE_ECHO + ---help--- + Test reading from popen() streams and closing them with pclose(). diff --git a/testing/libc/popen/Make.defs b/testing/libc/popen/Make.defs new file mode 100644 index 00000000000..7d7543b216e --- /dev/null +++ b/testing/libc/popen/Make.defs @@ -0,0 +1,25 @@ +############################################################################ +# apps/testing/libc/popen/Make.defs +# +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. The +# ASF licenses this file to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance with the +# License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. +# +############################################################################ + +ifneq ($(CONFIG_TESTING_POPEN_TEST),) +CONFIGURED_APPS += $(APPDIR)/testing/libc/popen +endif diff --git a/testing/libc/popen/Makefile b/testing/libc/popen/Makefile new file mode 100644 index 00000000000..9668f50bd64 --- /dev/null +++ b/testing/libc/popen/Makefile @@ -0,0 +1,34 @@ +############################################################################ +# apps/testing/libc/popen/Makefile +# +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. The +# ASF licenses this file to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance with the +# License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. +# +############################################################################ + +include $(APPDIR)/Make.defs + +# popen() test tool + +PROGNAME = popen_test +PRIORITY = SCHED_PRIORITY_DEFAULT +STACKSIZE = 4096 +MODULE = $(CONFIG_TESTING_POPEN_TEST) + +MAINSRC = popen_test.c + +include $(APPDIR)/Application.mk diff --git a/testing/libc/popen/popen_test.c b/testing/libc/popen/popen_test.c new file mode 100644 index 00000000000..6a7b8ccdef2 --- /dev/null +++ b/testing/libc/popen/popen_test.c @@ -0,0 +1,146 @@ +/**************************************************************************** + * apps/testing/libc/popen/popen_test.c + * + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. The + * ASF licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the + * License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + * + ****************************************************************************/ + +/**************************************************************************** + * Included Files + ****************************************************************************/ + +#include + +#include +#include +#include +#include + +/**************************************************************************** + * Pre-processor Definitions + ****************************************************************************/ + +#define POPEN_TEST_BUFLEN 64 + +/**************************************************************************** + * Private Functions + ****************************************************************************/ + +static int popen_test_read(FAR FILE *stream, FAR const char *expected) +{ + char buffer[POPEN_TEST_BUFLEN]; + + if (fgets(buffer, sizeof(buffer), stream) == NULL) + { + printf("popen_test: fgets failed: %d\n", errno); + return -1; + } + + if (strcmp(buffer, expected) != 0) + { + printf("popen_test: got \"%s\", expected \"%s\"\n", buffer, + expected); + return -1; + } + + return 0; +} + +static int popen_test_close(FAR FILE *stream) +{ + int ret; + + ret = pclose(stream); + if (ret < 0 && errno != ECHILD) + { + printf("popen_test: pclose failed: %d\n", errno); + return -1; + } + + return 0; +} + +/**************************************************************************** + * Public Functions + ****************************************************************************/ + +/**************************************************************************** + * Name: popen_test_main + ****************************************************************************/ + +int main(int argc, FAR char *argv[]) +{ + FAR FILE *stream; + FAR FILE *first; + FAR FILE *second; + + first = popen("echo popen_test_first", "r"); + if (first == NULL) + { + printf("popen_test: first popen failed: %d\n", errno); + return EXIT_FAILURE; + } + + second = popen("echo popen_test_second", "r"); + if (second == NULL) + { + printf("popen_test: second popen failed: %d\n", errno); + popen_test_close(first); + return EXIT_FAILURE; + } + + if (popen_test_read(first, "popen_test_first\n") < 0) + { + goto out_with_second; + } + + stream = first; + first = NULL; + if (popen_test_close(stream) < 0) + { + goto out_with_second; + } + + if (popen_test_read(second, "popen_test_second\n") < 0) + { + goto out_with_second; + } + + stream = second; + second = NULL; + if (popen_test_close(stream) < 0) + { + return EXIT_FAILURE; + } + + printf("popen_test: successful\n"); + return EXIT_SUCCESS; + +out_with_second: + if (second != NULL) + { + popen_test_close(second); + } + + if (first != NULL) + { + popen_test_close(first); + } + + return EXIT_FAILURE; +}