casaroli opened a new pull request, #3961:
URL: https://github.com/apache/nuttx-apps/pull/3961
## Summary
Since apache/nuttx#21324, a write to a stream socket that fails with `EPIPE`
raises `SIGPIPE`. With `CONFIG_SIG_SIGPIPE_ACTION`, the default action of
`SIGPIPE` ends the task. netutils/thttpd writes to the client socket in three
tasks: the server task (`thttpd_main()`), the CGI task (`cgi_child()`) and the
directory index task (`ls_child()`, with stdio). When a client closes the
connection during a transfer, the server stops for all clients, or the CGI task
stops before its clean-up.
This PR ignores `SIGPIPE` in the three tasks, as upstream thttpd does in
`main()` ("get EPIPE instead"). The failed write then ends only that
connection. `task_create()` starts each task with the default actions, so each
task sets it. The CGI task sets it after `exec()`, so the CGI program keeps the
default action.
`signal()` and not `send(..., MSG_NOSIGNAL)`: thttpd writes with `write()`
through `httpd_write()`, which also writes to the CGI pipes, and the index task
writes with `fprintf()`. `signal()` exists only with
`CONFIG_ENABLE_ALL_SIGNALS`, so the calls are in `#ifdef
CONFIG_ENABLE_ALL_SIGNALS`. `CONFIG_SIG_DEFAULT` and
`CONFIG_SIG_SIGPIPE_ACTION` are inside `if ENABLE_ALL_SIGNALS` in
sched/Kconfig, so without that option `SIGPIPE` cannot end a task and no call
is necessary.
The first commit fixes the older nxstyle errors in the three changed files,
because the CI check runs nxstyle on each changed file as a whole. It changes
only the layout: `git diff -w` shows only added braces, blank lines and two
wrapped lines. It also renames one local variable (`ths` to `server`) for
codespell.
The same change for other TCP servers: apache/nuttx-apps#3959, and
netutils/rexecd in apache/nuttx-apps#3957.
## Impact
Only netutils/thttpd. Without `CONFIG_SIG_SIGPIPE_ACTION` there is no change
in behaviour: a `SIGPIPE` without a handler is discarded, and the write already
returned the error. No documentation change:
Documentation/applications/netutils/thttpd does not describe this behaviour.
## Testing
qemu-armv8a:nsh (flat build, QEMU with HVF) on nuttx master 2eeaf9b6a33,
with `CONFIG_NET_TCP`, `NET_LOOPBACK`, `NET_TCP_WRITE_BUFFERS`, small socket
buffers (`NET_RECV_BUFSIZE` and `NET_SEND_BUFSIZE` 2048), `SIG_DEFAULT`,
`SIG_SIGPIPE_ACTION`, `FS_BINFS` and thttpd with BINFS CGI on 127.0.0.1:80. The
web root `/www` has a tmpfs at `/www/t` (`FS_PERMISSION`, so that the files are
not executable) and the binfs at `/www/cgi-bin`. A test program (builtin app)
starts thttpd in a task and runs two cases. A first client requests a URL,
reads 15 bytes and then resets the connection: it closes the socket with unread
data. A second client then requests `/t/index.html`. The URLs: a CGI (the test
program itself in CGI mode, which writes 300000 bytes to stdout and then
reports on stderr how much it wrote), then a 1 MB file.
To get `EPIPE` from TCP as POSIX gives it, the test applies a test-only
patch to nuttx (not part of any PR): the TCP send returns `EPIPE` instead of
`ENOTCONN` after a reset of the connection.
| Case | apps master b017038e5, TCP `EPIPE` | this PR, TCP `EPIPE` | apps
master, nuttx master | this PR, nuttx master |
|---|---|---|---|---|
| CGI: second client | PASS | PASS | PASS | PASS |
| CGI program after the reset | blocked forever in its write (the CGI task
ended by `SIGPIPE`) | wrote all 300006 bytes and exited | wrote all 300006
bytes | wrote all 300006 bytes |
| 1 MB file: second client | `ECONNREFUSED`, thttpd ended: FAIL | PASS |
PASS | PASS |
| ostest | 0 | 0 | 0 | 0 |
The last two columns show that nuttx master does not give the signal over
TCP today (`ENOTCONN`), and that the PR does not change that behaviour.
Not run: the directory index task (`CONFIG_THTTPD_GENERATE_INDICES`). A
request for a directory index crashes on master before the task writes
anything: `ls()` gives `task_create()` an argument list without the final NULL,
and with that fixed the index task still crashes in a corrupted semaphore. That
needs its own fix.
Builds with `EXTRAFLAGS="-Wno-cpp -Werror"`, no compiler warnings:
lincoln60:thttpd-binfs, olimex-lpc1766stk:thttpd-binfs (both with the GNU EABI
toolchain instead of buildroot), and lincoln60:thttpd-binfs with
`CONFIG_ENABLE_PARTIAL_SIGNALS` (the `signal()` calls compiled out).
qemu-armv8a:nsh with thttpd (the test build) shows only the older `%d` format
warnings of thttpd_alloc.c and thttpd_cgi.c on 64-bit, which are also on
master; arm64 builds with `-Werror`, so the test build used `-Wno-error=format`.
`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]
