casaroli opened a new pull request, #21100: URL: https://github.com/apache/nuttx/pull/21100
## Summary `tcsetpgrp()` passes a pointer to the process group ID to `TIOCSPGRP`, as POSIX and Linux do, and `include/nuttx/serial/tioctl.h` says so (`FAR const pid_t *`). The serial and the pty drivers read the argument as the ID itself: - a user address above 0x7fffffff (for example in an arm64 kernel build) gave `EINVAL`, so `tcsetpgrp()` always failed; - a lower address became the foreground process: the next `TIOCGPGRP` gave the address back, and Ctrl-C sent `SIGINT` to no task. This PR makes `TIOCSPGRP` read the ID through the pointer. A `NULL` pointer or an ID that is not positive gives `EINVAL`. `TIOCSCTTY` keeps its argument (a flag, or the PID itself), which NSH uses. The second commit fixes the serial driver page, which said that `TIOCSPGRP` takes the PID as a value. ## Impact `tcsetpgrp()` and `ioctl(TIOCSPGRP)` on the serial and the pty drivers, with `CONFIG_TTY_SIGINT` or `CONFIG_TTY_SIGTSTP`, in every build mode. `tcsetpgrp()` now works. A program that called `ioctl(fd, TIOCSPGRP, pid)` with the PID as a value must pass a pointer, as the header always said; no code in nuttx or nuttx-apps does this (NSH uses `TIOCSCTTY`). `TIOCSPGRP` with 0 no longer selects the calling task: it gives `EINVAL`, as for any ID that is not positive. ## Testing Tested on master aeab6fbf989 and on this PR rebased on it. Test program `tcpgrp.c` on `qemu-armv8a:knsh` (QEMU with HVF), with `CONFIG_PSEUDOTERM`, `CONFIG_PSEUDOTERM_SUSV1` and `CONFIG_TTY_SIGINT`: | Case | master | this PR | |---|---|---| | `tcsetpgrp(console, getpid())` | -1, `EINVAL`: FAIL | 0: PASS | | `tcgetpgrp(console)` | the PID (set by NSH): PASS | the PID: PASS | | `tcsetpgrp(console, 0)`, `tcsetpgrp(console, -5)` | -1: PASS | -1: PASS | | `tcsetpgrp(pty slave, getpid())` | -1, `EINVAL`: FAIL | 0: PASS | | `tcgetpgrp(pty slave)` | -1, `ENOTTY`: FAIL | the PID: PASS | ostest exits with status 0 on `qemu-armv8a:knsh` and `qemu-armv8a:nsh` (with the same options). Build with `CONFIG_TTY_SIGINT`, `CONFIG_TTY_SIGTSTP`, `CONFIG_SERIAL_TERMIOS`, `CONFIG_PSEUDOTERM` and `CONFIG_PSEUDOTERM_SUSV1`, each OK: `qemu-armv8a:nsh` (flat, arm64), `rv-virt:pnsh64` (protected, rv64), `rv-virt:knsh64` (kernel, rv64); `qemu-armv8a:knsh` (kernel, arm64) with the test options. Real hardware (on an earlier master, the same two commits), flat builds with `CONFIG_PSEUDOTERM`, `CONFIG_PSEUDOTERM_SUSV1`, `CONFIG_TTY_SIGINT`, `CONFIG_TTY_SIGTSTP` and `CONFIG_SERIAL_TERMIOS`: Pimoroni Pico Plus 2 (RP2350, Cortex-M33), `pimoroni-pico-2-plus:nsh`, and ESP32-S3-DevKitC (WROOM-2 N32R8V, Xtensa LX7), `esp32s3-devkit:nsh`. The tested tree has this PR and the other two serial changes (the Ctrl-C discard, the window size) together; `tcpgrp.c`, `winsz.c` and `isigtest.c` all run on it. ostest exits with status 0 on both boards (on the ESP32-S3 with `CONFIG_ESP32S3_DONT_USE_ROM_LIBC=y`: the ROM `strtoul()` fails the ostest strto test on master too). | Board | master | this PR | |---|---|---| | RP2350 | `tcsetpgrp()` returns 0 but stores the address of the PID: `tcgetpgrp()` gives 536894356 on the console and on the pty slave; `tcsetpgrp(fd, 0)` and `tcsetpgrp(fd, -5)` return 0: FAIL | 6/6 PASS | | ESP32-S3 | the same (`tcgetpgrp()` gives 1070154400): FAIL | 6/6 PASS | In a flat build master takes the address as the process group; in the kernel build above it returns `EINVAL`. `tools/checkpatch.sh -c -u -m -g` passes. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
