summaryrefslogtreecommitdiffhomepage
AgeCommit message (Collapse)AuthorFilesLines
2023-05-08Docs: documenting the renamed directory flags in unitd.8.Alejandro Colomar1-4/+4
Reviewed-by: Artem Konev <a.konev@f5.com> Signed-off-by: Alejandro Colomar <alx@nginx.com>
2023-05-08Docs: not using shouty caps in unitd.8.Alejandro Colomar1-10/+10
There was a recent unanimous agreement by maintainers of groff, mandoc, the Linux man-pages, and other relevant programmers, that manual pages should not use uppercase unnecessarily. Use of uppercase in the title and in the section's titles dates from before one could use bold, italics, and other such formatting, so that it was the way of giving more importance to certain parts of a page. Nowadays, we use bold, so uppercase is unnecessary. Moreover, using uppercase in the title is bad, since it removes information. If we keep the exact casing used in the program (or function) name, we provide more information. And anyway, if users want to read in uppercase, they can program certain mdoc(7) or man(7) macros to transform their arguments into uppercase. This could be done via </etc/groff/mdoc.local> and </etc/groff/man.local>. There's a plan of transforming OpenBSD pages and the Linux man-pages to stop using uppercase. Other projects may join. That will likely happen in the following months. Let's align with this. Reviewed-by: Artem Konev <a.konev@f5.com> Signed-off-by: Alejandro Colomar <alx@nginx.com>
2023-05-08Docs: removed '-v' from unitd.8.Alejandro Colomar1-2/+2
This short option is not really supported. Probably it was just a typo. Reviewed-by: Artem Konev <a.konev@f5.com> Signed-off-by: Alejandro Colomar <alx@nginx.com>
2023-05-08Docs: moved uintd.8 to man8/ subdirectory.Alejandro Colomar5-6/+6
Reviewed-by: Artem Konev <a.konev@f5.com> Signed-off-by: Alejandro Colomar <alx@nginx.com>
2023-04-12Docker: made curl fail with non-zero exit code on server errors.Konstantin Pavlov1-1/+1
2023-04-25Allow to remove the version string in HTTP responses.Andrew Clayton4-3/+22
Normally Unit responds to HTTP requests by including a header like Server: Unit/1.30.0 however it can sometimes be beneficial to withhold the version information and in this case just respond with Server: Unit This patch adds a new "settings.http" boolean option called server_version, which defaults to true, in which case the full version information is sent. However this can be set to false, e.g "settings": { "http": { "server_version": false } }, in which case Unit responds without the version information as the latter example above shows. Link: <https://www.ietf.org/rfc/rfc9110.html#section-10.2.4> Closes: <https://github.com/nginx/unit/issues/158> Reviewed-by: Alejandro Colomar <alx@nginx.com> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
2023-04-25Decouple "Unit" from NXT_SERVER.Andrew Clayton1-1/+2
Split out the "Unit" name from the NXT_SERVER #define into its own NXT_NAME #define, then make NXT_SERVER a combination of that and NXT_VERSION. This is required for a subsequent commit where we may want the server name on its own. Reviewed-by: Alejandro Colomar <alx@nginx.com> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
2023-04-24Remove an erroneous semi-colon.Andrew Clayton1-1/+1
Reviewed-by: Alejandro Colomar <alx@nginx.com> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
2023-04-24Don't conflate the error variable in nxt_kqueue_poll().Andrew Clayton1-3/+4
In nxt_kqueue_poll() error is declared as a nxt_bool_t aka unsigned int (on x86-64 anyway). It is used both as a boolean and as the return storage for a bitwise AND operation. This has potential to go awry. If nxt_bool_t was changed to be a u8 then we would have the following issue gcc12 -c -pipe -fPIC -fvisibility=hidden -O -W -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wmissing-prototypes -Werror -g -O2 -I src -I build -I/usr/local/include -o build/src/nxt_kqueue_engine.o -MMD -MF build/src/nxt_kqueue_engine.dep -MT build/src/nxt_kqueue_engine.o src/nxt_kqueue_engine.c src/nxt_kqueue_engine.c: In function 'nxt_kqueue_poll': src/nxt_kqueue_engine.c:728:17: error: overflow in conversion from 'int' to 'nxt_bool_t' {aka 'unsigned char'} changes value from '(int)kev->flags & 16384' to '0' [-Werror=overflow] 728 | error = (kev->flags & EV_ERROR); | ^ cc1: all warnings being treated as errors EV_ERROR has the value 16384, after the AND operation error holds 16384, however this overflows and wraps around (64 times) exactly to 0. With nxt_bool_t defined as a u32, we would have a similar issue if EV_ERROR ever became UINT_MAX + 1 (or a multiple thereof)... Rather than conflating the use of error, keep error as a boolean (it is used further down the function) but do the AND operation inside the if (). Reviewed-by: Alejandro Colomar <alx@nginx.com> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
2023-04-24Remove a bunch of dead code.Andrew Clayton22-4948/+0
This removes a bunch of unused files that would have been touched by subsequent commits that switch to using nxt_bool_t (AKA unit6_t) in structures. In auto/sources we have NXT_LIB_SRC0=" \ src/nxt_buf_filter.c \ src/nxt_job_file.c \ src/nxt_stream_module.c \ src/nxt_stream_source.c \ src/nxt_upstream_source.c \ src/nxt_http_source.c \ src/nxt_fastcgi_source.c \ src/nxt_fastcgi_record_parse.c \ \ src/nxt_mem_pool_cleanup.h \ src/nxt_mem_pool_cleanup.c \ " None of these seem to actually be used anywhere (other than within themselves). That variable is _not_ referenced anywhere else. Also remove the unused related header files: src/nxt_buf_filter.h, src/nxt_fastcgi_source.h, src/nxt_http_source.h, src/nxt_job_file.h, src/nxt_stream_source.h and src/nxt_upstream_source.h Also, these files do not seem to be used, no mention under auto/ or build/ src/nxt_file_cache.c src/nxt_cache.c src/nxt_job_file_cache.c src/nxt_cache.h is #included in src/nxt_main.h, but AFAICT is not actually used. With all the above removed $ ./configure --openssl --debug --tests && make -j && make -j tests && make libnxt all builds. Buildbot passes. NOTE: You may need to do a 'make clean' before the next build attempt. Reviewed-by: Alejandro Colomar <alx@nginx.com> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
2023-04-15Tools: setup-unit: unified repeated code.Alejandro Colomar1-9/+7
Instead of doing the same operation in each subcommand, do it once in the parent. Reviewed-by: Andrew Clayton <a.clayton@nginx.com> Signed-off-by: Alejandro Colomar <alx@nginx.com>
2023-04-11contrib: updated njs to 0.7.12.Konstantin Pavlov3-3/+6
2023-04-12HTTP: optimizing $request_line.Alejandro Colomar6-45/+24
Don't reconstruct a new string for the $request_line from the parsed method, target, and HTTP version, but rather keep a pointer to the original memory where the request line was received. This will be necessary for implementing URI rewrites, since we want to log the original request line, and not one constructed from the rewritten target. This implementation changes behavior (only for invalid requests) in the following way: Previous behavior was to log as many tokens from the request line as were parsed validly, thus: Request -> access log ; error log "GET / HTTP/1.1" -> "GET / HTTP/1.1" OK ; = "GET / HTTP/1.1" -> "GET / HTTP/1.1" [1] ; = "GET / HTTP/2.1" -> "GET / HTTP/2.1" OK ; = "GET / HTTP/1." -> "GET / HTTP/1." [2] ; "GET / HTTP/1. [null]" "GET / food" -> "GET / food" [2] ; "GET / food [null]" "GET / / HTTP/1.1" -> "GET / / HTTP/1.1" [2] ; = "GET / / HTTP/1.1" -> "GET / / HTTP/1.1" [2] ; = "GET food HTTP/1.1" -> "GET" ; "GET [null] [null]" "OPTIONS * HTTP/1.1" -> "OPTIONS" [3] ; "OPTIONS [null] [null]" "FOOBAR baz HTTP/1.1"-> "FOOBAR" ; "FOOBAR [null] [null]" "FOOBAR / HTTP/1.1" -> "FOOBAR / HTTP/1.1" ; = "get / HTTP/1.1" -> "-" ; " [null] [null]" "" -> "-" ; " [null] [null]" This behavior was rather inconsistent. We have several options to go forward with this patch: - NGINX behavior. Log the entire request line, up to '\r' | '\n', even if it was invalid. This is the most informative alternative. However, RFC-complying requests will probably not send invalid requests. This information would be interesting to users where debugging requests constructed manually via netcat(1) or a similar tool, or maybe for debugging a client, are important. It might be interesting to support this in the future if our users are interested; for now, since this approach requires looping over invalid requests twice, that's an overhead that we better avoid. - Previous Unit behavior This is relatively fast (almost as fast as the next alternative, the one we chose), but the implementation is ugly, in that we need to perform the same operation in many places around the code. If we want performance, probably the next alternative is better; if we want to be informative, then the first one is better (maybe in combination with the third one too). - Chosen behavior Only logging request lines when the request is valid. For any invalid request, or even unsupported ones, the request line will be logged as "-". Thus: Request -> access log [4] "GET / HTTP/1.1" -> "GET / HTTP/1.1" OK "GET / HTTP/1.1" -> "GET / HTTP/1.1" [1] "GET / HTTP/2.1" -> "-" [3] "GET / HTTP/1." -> "-" "GET / food" -> "-" "GET / / HTTP/1.1" -> "GET / / HTTP/1.1" [2] "GET / / HTTP/1.1" -> "GET / / HTTP/1.1" [2] "GET food HTTP/1.1" -> "-" "OPTIONS * HTTP/1.1" -> "-" "FOOBAR baz HTTP/1.1"-> "-" "FOOBAR / HTTP/1.1" -> "FOOBAR / HTTP/1.1" "get / HTTP/1.1" -> "-" "" -> "-" This is less informative than previous behavior, but considering how inconsistent it was, and that RFC-complying agents will probably not send us such requests, we're ready to lose that information in the log. This is of course the fastest and simplest implementation we can get. We've chosen to implement this alternative in this patch. Since we modified the behavior, this patch also changes the affected tests. [1]: Multiple successive spaces as a token delimiter is allowed by the RFC, but it is discouraged, and considered a security risk. It is currently supported by Unit, but we will probably drop support for it in the future. [2]: Unit currently supports spaces in the request-target. This is a violation of the relevant RFC (linked below), and will be fixed in the future, and consider those targets as invalid, returning a 400 (Bad Request), and thus the log lines with the previous inconsistent behavior would be changed. [3]: Not yet supported. [4]: In the error log, regarding the "log_routes" conditional logging of the request line, we only need to log the request line if it was valid. It doesn't make sense to log "" or "-" in case that the request was invalid, since this is only useful for understanding decisions of the router. In this case, the access log is more appropriate, which shows that the request was invalid, and a 400 was returned. When the request line is valid, it is printed in the error log exactly as in the access log. Link: <https://datatracker.ietf.org/doc/html/rfc9112#section-3> Suggested-by: Liam Crilly <liam@nginx.com> Reviewed-by: Zhidao Hong <z.hong@f5.com> Cc: Timo Stark <t.stark@nginx.com> Cc: Andrei Zeliankou <zelenkov@nginx.com> Cc: Andrew Clayton <a.clayton@nginx.com> Cc: Artem Konev <a.konev@f5.com> Signed-off-by: Alejandro Colomar <alx@nginx.com>
2023-04-11Docker: fixed a typo.Konstantin Pavlov1-1/+1
2023-04-11Add per-application logging.Andrew Clayton5-0/+81
Currently when running in the foreground, unit application processes will send stdout to the current TTY and stderr to the unit log file. That behaviour won't change. When running as a daemon, unit application processes will send stdout to /dev/null and stderr to the unit log file. This commit allows to alter the latter case of unit running as a daemon, by allowing applications to redirect stdout and/or stderr to specific log files. This is done via two new application options, 'stdout' & 'stderr', e.g "applications": { "myapp": { ... "stdout": "/path/to/log/unit/app/stdout.log", "stderr": "/path/to/log/unit/app/stderr.log" } } These log files are created by the application processes themselves and thus the log directories need to be writable by the user (and or group) of the application processes. E.g $ sudo mkdir -p /path/to/log/unit/app $ sudo chown APP_USER /path/to/log/unit/app These need to be setup before starting unit with the above config. Currently these log files do not participate in log-file rotation (SIGUSR1), that may change in a future commit. In the meantime these logs can be rotated using the traditional copy/truncate method. NOTE: You may or may not see stuff printed to stdout as stdout was traditionally used by CGI applications to communicate with the webserver. Closes: <https://github.com/nginx/unit/issues/197> Closes: <https://github.com/nginx/unit/issues/846> Reviewed-by: Alejandro Colomar <alx@nginx.com> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
2023-04-11Add nxt_file_stdout().Andrew Clayton2-0/+20
This is analogous to the nxt_file_stderr() function and will be used in a subsequent commit. This function redirects stdout to a given file descriptor. Reviewed-by: Alejandro Colomar <alx@nginx.com> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
2023-04-11PHP: Make the filter_input() function work.Andrew Clayton1-3/+12
On GitHub, @jamesRUS52 reported that the PHP filter_input()[0] function would just return NULL. To enable this function we need to run the variables through the sapi_module.input_filter() function when we call php_register_variable_safe(). In PHP versions prior to 7.0.0, input_filter() takes 'len' as an unsigned int, while later versions take it as a size_t. Now, with this commit and the following PHP <?php var_dump(filter_input(INPUT_SERVER, 'REMOTE_ADDR')); var_dump(filter_input(INPUT_SERVER, 'REQUEST_URI')); var_dump(filter_input(INPUT_GET, 'get', FILTER_SANITIZE_SPECIAL_CHARS)); ?> you get $ curl 'http://localhost:8080/854.php?get=foo<>' string(3) "::1" string(18) "/854.php?get=foo<>" string(13) "foo&#60;&#62;" [0]: <https://www.php.net/manual/en/function.filter-input.php> Tested-by: <https://github.com/jamesRUS52> Closes: <https://github.com/nginx/unit/issues/854> Reviewed-by: Alejandro Colomar <alx@nginx.com> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
2023-03-28Docker: fixed git references.Konstantin Pavlov1-2/+2
2023-04-06Docker: drop apt-get clean usage.Konstantin Pavlov1-1/+1
It's automatic in the Debian and Ubuntu containers: https://github.com/debuerreotype/debuerreotype/blob/5cf7949ecf1cec1afece267688bda64cd34a6817/scripts/debuerreotype-minimizing-config#L85-L109
2023-04-06Docker: explicitely set uid/gid to 999 for unit user.Konstantin Pavlov1-2/+2
This allows us to be consistent through possible updates of default settings used in distributions. Previous behaviour was uid/gid were chosen automatically based on what uids/gids are already taken on the system.
2023-04-06Packages: use groupadd/useradd on Debian-based operating systems.Konstantin Pavlov2-10/+8
addgroup/adduser will no longer be installed by default in the "minbase". Also, moving to lower-level utilities saves us one runtime dependency.
2023-04-06Docker: added OCI image-spec labels.Konstantin Pavlov1-1/+7
2023-04-06Docker: specified explicit variants of images to use.Konstantin Pavlov1-8/+17
This allows us to decide when to move to a newer underlying distribution version with our pace instead of relying on Docker Hub cadence.
2023-04-06Docker: dropped a leftover from a multi-stage build.Konstantin Pavlov1-1/+1
2023-04-10Docker: check out packaging tags.Konstantin Pavlov2-2/+5
This will ensure we're checking out source code that is close to what we have in binary packages. While at it, remove the checkout directory when it's no longer needed.
2023-03-30Docker: added njs support.Konstantin Pavlov1-4/+8
2023-03-30Packages: added unitc and setup-unit.Konstantin Pavlov5-2/+15
2023-04-04Tools: use control socket and log file from running instance.Liam Crilly1-2/+2
If unitd was started with an explicit path then unitc will use that binary instead of the default PATH to obtain the default control socket and log file locations.
2023-04-03Remove a useless assignment in nxt_mem_zone_alloc_pages().Andrew Clayton1-1/+1
This was reported by the 'Clang Static Analyzer' as a 'dead nested assignment'. We assign prev_size then check if it's != 0 and if true we then set prev_pages to page_size right shifted by two at the same time setting prev_size to be right shifted by two (>>=), however page_size is never used again so no need to set it here. Reviewed-by: Alejandro Colomar <alx@nginx.com> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
2023-04-03Prevent a possible NULL de-reference in nxt_job_create().Andrew Clayton1-4/+6
We allocate 'job' we then have a check if it's not NULL and do stuff with it, but then we accessed it outside this check. Simply return if job is NULL. Reviewed-by: Alejandro Colomar <alx@nginx.com> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
2023-04-03Remove a useless assignment in nxt_fs_mkdir_all().Andrew Clayton1-1/+1
This was reported by the 'Clang Static Analyzer' as a 'dead nested assignment'. We set end outside the loop but the first time we use it is to assign it in the loop (not used anywhere else). Further cleanup could be to reduce the scope of end by moving its declaration inside the loop. Reviewed-by: Alejandro Colomar <alx@nginx.com> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
2023-03-30Packages: Used a stricter check for Amazon Linux 2023.Konstantin Pavlov1-2/+2
Previously, findstring matched on amazonlinux2 too, breaking the build on that OS.
2023-03-29Packages: fixed rpm builds after 817968931c58.Konstantin Pavlov1-3/+3
2023-03-22Packages: Added Amazon Linux 2023.Konstantin Pavlov6-1/+108
2023-03-22Packages: check rpm database for actual provides.Konstantin Pavlov1-1/+1
Previously, we required an exact non-virtual package, however it's fine if some package has a fully-virtual provides for what we need.
2023-03-29Auto: mirroring installation structure in build tree.Alejandro Colomar17-113/+122
This makes the build tree more organized, which is good for adding new stuff. Now, it's useful for example for adding manual pages in man3/, but it may be useful in the future for example for extending the build system to run linters (e.g., clang-tidy(1), Clang analyzer, ...) on the C source code. Previously, the build tree was quite flat, and looked like this (after `./configure && make`): $ tree -I src build build ├── Makefile ├── autoconf.data ├── autoconf.err ├── echo ├── libnxt.a ├── nxt_auto_config.h ├── nxt_version.h ├── unitd └── unitd.8 1 directory, 9 files And after this patch, it looks like this: $ tree -I src build build ├── Makefile ├── autoconf.data ├── autoconf.err ├── bin │ └── echo ├── include │ ├── nxt_auto_config.h │ └── nxt_version.h ├── lib │ ├── libnxt.a │ └── unit │ └── modules ├── sbin │ └── unitd ├── share │ └── man │ └── man8 │ └── unitd.8 └── var ├── lib │ └── unit ├── log │ └── unit └── run └── unit 17 directories, 9 files It also solves one issue introduced in 5a37171f733f ("Added default values for pathnames."). Before that commit, it was possible to run unitd from the build system (`./build/unitd`). Now, since it expects files in a very specific location, that has been broken. By having a directory structure that mirrors the installation, it's possible to trick it to believe it's installed, and run it from there: $ ./configure --prefix=./build $ make $ ./build/sbin/unitd Fixes: 5a37171f733f ("Added default values for pathnames.") Reported-by: Liam Crilly <liam@nginx.com> Reviewed-by: Konstantin Pavlov <thresh@nginx.com> Reviewed-by: Andrew Clayton <a.clayton@nginx.com> Cc: Andrei Zeliankou <zelenkov@nginx.com> Cc: Zhidao Hong <z.hong@f5.com> Signed-off-by: Alejandro Colomar <alx@nginx.com>
2023-03-29Renamed --libstatedir to --statedir.Alejandro Colomar11-22/+22
In BSD systems, it's usually </var/db> or some other dir under </var> that is not </var/lib>, so $statedir is a more generic name. See hier(7). Reported-by: Andrei Zeliankou <zelenkov@nginx.com> Reported-by: Zhidao Hong <z.hong@f5.com> Reviewed-by: Konstantin Pavlov <thresh@nginx.com> Reviewed-by: Andrew Clayton <a.clayton@nginx.com> Cc: Liam Crilly <liam@nginx.com> Signed-off-by: Alejandro Colomar <alx@nginx.com>
2023-03-27Tests: relaxed jar glob.Konstantin Pavlov1-1/+1
We install jars with names like websocket-api-${NXT_JAVA_MODULE}-$NXT_VERSION.jar, which translates to versioned NXT_JAVA_MODULE in the packaging system, e.g. websocket-api-java11-1.30.0.jar.
2023-02-13Docker: bumped language versions.Konstantin Pavlov1-3/+3
2023-02-13Docker: limited the waiting time for control socket creation.Konstantin Pavlov1-2/+9
While at it, fixed a typo.
2023-02-13Docker: made dockerfiles use a single stage build process.Konstantin Pavlov2-35/+22
2023-02-13Docker: added a target to generate Docker library definition.Konstantin Pavlov1-1/+18
2023-02-13Docker: cleanup unused targets.Konstantin Pavlov1-20/+2
2023-03-21Tests: added tests for the "log_route" option.Andrei Zeliankou1-0/+154
2023-03-21HTTP: added route logging.Alejandro Colomar8-0/+43
- Configuration: added "/config/settings/http/log_route". Type: bool Default: false This adds configurability to the error log. It allows enabling and disabling logs related to how the router performs selection of the routes. - HTTP: logging request line. Log level: [notice] The request line is essential to understand which logs correspond to which request when reading the logs. - HTTP: logging route that's been discarded. Log level: [info] - HTTP: logging route whose action is selected. Log level: [notice] - HTTP: logging when "fallback" action is taken. Log level: [notice] Closes: <https://github.com/nginx/unit/issues/758> Link: <https://github.com/nginx/unit/pull/824> Link: <https://github.com/nginx/unit/pull/839> Suggested-by: Timo Stark <t.stark@nginx.com> Suggested-by: Mark L Wood-Patrick <mwoodpatrick@gmail.com> Suggested-by: Liam Crilly <liam@nginx.com> Tested-by: Liam Crilly <liam@nginx.com> Acked-by: Artem Konev <a.konev@f5.com> Cc: Andrew Clayton <a.clayton@nginx.com> Cc: Andrei Zeliankou <zelenkov@nginx.com> Reviewed-by: Zhidao Hong <z.hong@f5.com> Signed-off-by: Alejandro Colomar <alx@nginx.com>
2023-03-21HTTP: rewrote while loop as for loop.Alejandro Colomar1-7/+3
This considerably simplifies the function, and will also help log the iteration in which we are, which corresponds to the route array element. Link: <https://github.com/nginx/unit/pull/824> Link: <https://github.com/nginx/unit/pull/839> Reviewed-by: Andrew Clayton <a.clayton@nginx.com> Tested-by: Liam Crilly <liam@nginx.com> Reviewed-by: Zhidao Hong <z.hong@f5.com> Signed-off-by: Alejandro Colomar <alx@nginx.com>
2023-03-17Default PR_SET_NO_NEW_PRIVS to off.Andrew Clayton1-0/+4
This prctl(2) option was enabled in commit 0277d8f1 ("Isolation: Fix the enablement of PR_SET_NO_NEW_PRIVS.") and this was being set by default. This prctl(2) when enabled renders (amongst other things) the set-UID and set-GID bits on executables ineffective after an execve(2). This causes an issue for applications that want to execute the sendmail(8) binary, this includes the PHP mail() function, which is usually set-GID. After some internal discussion it was decided to disable this option by default. Closes: <https://github.com/nginx/unit/issues/852> Fixes: 0277d8f1 ("Isolation: Fix the enablement of PR_SET_NO_NEW_PRIVS.") Fixes: e2b53e16 ("Added "rootfs" feature.") Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
2023-03-17Improve an error message regarding Unix domain sockets.Andrew Clayton1-1/+1
When starting unit, if its Unix domain control socket was already active you would get an error message like 2023/03/15 18:07:55 [alert] 53875#8669650 connect(5, unix:/tmp/control.sock) succeed, address already in use which is confusing in a couple of regards, firstly we have the classic success/failure message and secondly 'address already in use' is an actual errno value, EADDRINUSE and we didn't get an error from this connect(2). Re-word this error message for greater clarity. Reported-by: Liam Crilly <liam.crilly@nginx.com> Cc: Liam Crilly <liam.crilly@nginx.com> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
2023-03-17Socket: Remove Unix domain listen sockets upon reconfigure.Andrew Clayton3-3/+87
Currently when using Unix domain sockets for requests, if unit is reconfigured then it will fail if it tries to bind(2) again to a Unix domain socket with something like 2023/02/25 19:15:50 [alert] 35274#35274 bind(\"unix:/tmp/unit.sock\") failed (98: Address already in use) When closing such a socket we really need to unlink(2) it. However that presents a problem in that when running as root, while the main process runs as root and creates the socket, it's the router process, that runs as an unprivileged user, e.g nobody, that closes the socket and would thus remove it, but couldn't due to not having permission, even if the socket is mode 0666, you need write permissions on the containing directory to remove a file. There are several options to solve this, all with varying degrees of complexity and utility. 1) Give the user who the router process runs as write permission to the directory containing the listen sockets. These can then be unlink(2)'d from the router process. Simple and would work, but perhaps not the most elegant. 2) Using capabilities(7). The router process could temporarily attain the CAP_DAC_OVERRIDE capability, unlink(7) the socket, then relinquish the capability until required again. These are Linux specific (other systems may have similar mechanisms which would be extra work to support). There is also a, albeit small, window where the router process is running with elevated privileges. 3) Have the main process do the unlink(2), it is after all the process that created the socket. This is what this commit implements. We create a new port IPC message type of NXT_PORT_MSG_SOCKET_UNLINK, that is used by the router process to notify the main process about a Unix domain socket to unlink(2). Upon doing a reconfigure the router process will call nxt_router_listen_socket_release() which will close the socket, we extend this function in the case of non-abstract Unix domain sockets, so that it will send a message to the main process containing a copy of the nxt_sockaddr_t structure that will contain the filename of the socket. In the main process the handler that we have defined, nxt_main_port_socket_unlink_handler(), for this message type will run and allow us to look for the socket in question in the listen_sockets array and remove it and unlink(2) the socket. This then allows the reconfigure to work if it tries to bind(2) again to a socket that previously existed. Link: <https://github.com/nginx/unit/issues/669> Link: <https://github.com/nginx/unit/pull/735> Reviewed-by: Alejandro Colomar <alx@nginx.com> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
2023-03-17Remove some dormant code from nxt_process_quit().Andrew Clayton1-21/+0
In nxt_process_quit() there is a loop that iterates over the task->thread->runtime->listen_sockets array and closes the connections. This code has been there from the beginning $ git log --pretty=oneline -S'if (rt->listen_sockets != NULL)' e9e5ddd5a5d9ce99768833137eac2551a710becf Refactor of process management. 6f2c9acd1841ca20a1388b34aef64e9f00459090 Processes refactoring. The cycle has been renamed to the runtime. $ git log --pretty=oneline -S'if (cycle->listen_sockets != NULL) {' 6f2c9acd1841ca20a1388b34aef64e9f00459090 Processes refactoring. The cycle has been renamed to the runtime. 16cbf3c076a0aca6d47adaf3f719493674cf2363 Initial version. but never seems to have been used (AFAICT and certainly not recently, confirmed by code inspection and running pytests with a bunch of language modules enabled and the code in question was never executed) as the listen_sockets array has never been populated... until now. The previous commit now adds Unix domain sockets to this array so that they can be unlink(2)'d upon exit and reconfiguration. This has now caused this dormant code to become active as it now tries to close these sockets (from at least the prototype processes), this array is inherited via fork by other processes. The file descriptor for these sockets is set to -1 when they are put into this array. This then results in close(-1) calls which caused multiple failures in the pytests such as > assert not alerts, 'alert(s)' E AssertionError: alert(s) E assert not ['2023/03/09 23:26:14 [alert] 137673#137673 socket close(-1) failed (9: Bad file descriptor)'] I think the simplest thing is to just remove this code. Reviewed-by: Alejandro Colomar <alx@nginx.com> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>