From fd937d1d3fe4b3f0fc06fc1f8c1506b1b95f1279 Mon Sep 17 00:00:00 2001 From: Nightt <87569709+nightt5879@users.noreply.github.com> Date: Sat, 30 May 2026 13:06:47 +0800 Subject: [PATCH 1/2] system/popen: Avoid copying FILE Store the popen fd and shell pid in a fopencookie private cookie instead of embedding a copied FILE object in the popen container. The stream callbacks forward I/O to the pipe fd, and pclose() reads the shell pid from the stream cookie before fclose() closes the fd. Fixes #2937. Signed-off-by: Nightt <87569709+nightt5879@users.noreply.github.com> --- system/popen/popen.c | 116 ++++++++++++++++++++++++++++++++----------- 1 file changed, 87 insertions(+), 29 deletions(-) diff --git a/system/popen/popen.c b/system/popen/popen.c index b52fbaafe85..3b8a62203a8 100644 --- a/system/popen/popen.c +++ b/system/popen/popen.c @@ -45,17 +45,65 @@ * 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; + int fd; pid_t shell; }; +/**************************************************************************** + * Private Functions + ****************************************************************************/ + +/**************************************************************************** + * Name: popen_file_read + ****************************************************************************/ + +static ssize_t popen_file_read(FAR void *cookie, FAR char *buf, + size_t size) +{ + FAR struct popen_file_s *filep = (FAR struct popen_file_s *)cookie; + + return read(filep->fd, buf, size); +} + +/**************************************************************************** + * Name: popen_file_write + ****************************************************************************/ + +static ssize_t popen_file_write(FAR void *cookie, FAR const char *buf, + size_t size) +{ + FAR struct popen_file_s *filep = (FAR struct popen_file_s *)cookie; + + return write(filep->fd, buf, size); +} + +/**************************************************************************** + * Name: popen_file_seek + ****************************************************************************/ + +static off_t popen_file_seek(FAR void *cookie, FAR off_t *offset, + int whence) +{ + set_errno(ESPIPE); + return ERROR; +} + +/**************************************************************************** + * Name: popen_file_close + ****************************************************************************/ + +static int popen_file_close(FAR void *cookie) +{ + FAR struct popen_file_s *filep = (FAR struct popen_file_s *)cookie; + int ret; + + ret = close(filep->fd); + free(filep); + return ret; +} + /**************************************************************************** * Public Functions ****************************************************************************/ @@ -115,6 +163,15 @@ struct popen_file_s FILE *popen(FAR const char *command, FAR const char *mode) { FAR struct popen_file_s *container; + FAR FILE *stream; + cookie_io_functions_t popen_io = + { + .read = popen_file_read, + .write = popen_file_write, + .seek = popen_file_seek, + .close = popen_file_close + }; + struct sched_param param; posix_spawnattr_t attr; posix_spawn_file_actions_t file_actions; @@ -193,8 +250,9 @@ 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->fd = retfd; + stream = fopencookie(container, mode, popen_io); + if (stream == NULL) { errcode = errno; goto errout_with_pipe; @@ -329,10 +387,7 @@ FILE *popen(FAR const char *command, FAR const char *mode) ioctl(retfd, FIOCLEX, 0); } - /* Finale and return input input/output stream */ - - memcpy(&container->copy, container->original, sizeof(FILE)); - return &container->copy; + return stream; errout_with_actions: posix_spawn_file_actions_destroy(&file_actions); @@ -341,14 +396,18 @@ FILE *popen(FAR const char *command, FAR const char *mode) posix_spawnattr_destroy(&attr); errout_with_stream: - fclose(container->original); + fclose(stream); + container = NULL; errout_with_pipe: close(fd[0]); close(fd[1]); errout_with_container: - free(container); + if (container != NULL) + { + free(container); + } errout: errno = errcode; @@ -399,31 +458,30 @@ 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) - */ + if (stream == NULL || stream->fs_iofunc.close != popen_file_close) + { + errno = EINVAL; + return ERROR; + } - fclose(original); + container = (FAR struct popen_file_s *)stream->fs_cookie; + if (container == NULL) + { + errno = EINVAL; + return ERROR; + } shell = container->shell; - free(container); + fclose(stream); #ifdef CONFIG_SCHED_WAITPID /* Wait for the shell to exit, retrieving the return value if available. */ From 9549d3bfc82100a7b9bb92fd604f9281bc40af5f Mon Sep 17 00:00:00 2001 From: Nightt <87569709+nightt5879@users.noreply.github.com> Date: Sat, 30 May 2026 13:07:00 +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 independent popen streams and pclose() handling for multiple concurrently opened popen streams. 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; +}