This allows to configure the Wasm module, e.g
./configure wasm --include-path=/path/to/wasmtime-v11.0.0-x86_64-linux-c-api/include --lib-path=/path/to/wasmtime-v11.0.0-x86_64-linux-c-api/lib --rpath
--rpath as above says to set the rpath to the value of --lib-path. You
can alternatively specify a directory to use as the rpath. Or simply
omit the option to not have an rpath set.
This is mostly useful for during development where you may not have the
Wasmtime stuff installed to system directories or you want to test with
newer/different versions.
See ./configure wasm --help for a full list of options.
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This adds the core of runtime WebAssembly[0] support. Future commits
will enable this in the Unit core and expose the configuration.
This introduces a new src/wasm directory for storing this source.
We are initially using Wasmtime[0] as the WebAssembly runtime, however
this has been designed with the ability to use different runtimes in
mind.
src/wasm/nxt_wasm.[ch] is the main interface to Unit.
src/wasm/nxt_rt_wasmtime.c is the Wasmtime runtime support. This is
nicely insulated from any knowledge of internal Unit workings.
Wasmtime is what loads and runs the Wasm modules.
The Wasm modules can export functions Wasmtime can call and Wasmtime can
export functions that the module can call.
We make use of both. The terminology used is that function exports are
what the Wasm module exports and function imports are what the Wasm
runtime exports to the module.
We currently have four function imports (functions exported by the
runtime to be called by the Wasm module).
1) nxt_wasm_get_init_mem_size
This allows Wasm modules to get the size of the initially allocated
shared memory. This is the size allocated at Unit startup and what the
Wasm modules can assume they have access to (in reality this shared
memory will likely be larger).
The amount of memory allocated at startup is NXT_WASM_MEM_SIZE which as
of this commit is 32MiB.
We do actually allocate NXT_WASM_MEM_SIZE + NXT_WASM_PAGE_SIZE at
startup which is an extra 64KiB (the smallest allocation unit), this is
to allow room for the response structure and so module developers can
just assume they have the full 32MiB for their actual response.
2) nxt_wasm_send_headers
This allows WASM modules to send their headers.
3) nxt_wasm_send_response
This allows WASM modules to send their response.
4) nxt_wasm_response_end
This allows WASM modules to inform Unit they have finished sending their
response. This calls nxt_unit_request_done()
Then there are currently up to eight functions that a module can export.
Three of which are required. These function can be named anything. I'll
use the Unit configuration names to refer to them
1) request_handler
The main driving function. This may be called multiple times for a
single HTTP request if the request is larger than the shared memory.
2) malloc_handler
Used to allocate a chunk of memory at language module startup. This
memory is allocated from the WASM modules address space and is what is
sued for communicating between the WASM module (the guest) and Unit (the
host).
3) free_handler
Used to free the memory from above at language module shutdown.
Then there are the following optional handlers
1) module_init_handler
If set, called at language module startup.
2) module_end_handler
If set, called at language module shutdown.
3) request_init_handler
If set, called at the start of request. Called only once per HTTP
request.
4) request_end_handler
If set, called once all of a request has been sent to the WASM module.
5) response_end_handler
If set, called at the end of a request, once the WASM module has sent
all its headers and data.
32bits
We currently support 32bit WASM modules, I.e wasm32-wasi. Newer version
of clang, 13+[2], do seem to have support for wasm64 as a target (which
uses a LP64 model). However it's not entirely clear if the WASI SDK
fully supports[3] this and by extension WASI libc/wasi-sysroot.
64bit support is something than can be explored more thoroughly in the
future.
As such in structures that are used to communicate between the host and
guest we use 32bit ints. Even when a single byte might be enough. This
is to avoid issues with structure layout differences between a 64bit
host and 32bit guest (I.e WASM module) and the need for various bits of
structure padding depending on host architecture. Instead everything is
4-byte aligned.
[0]: <https://webassembly.org/>
[1]: <https://wasmtime.dev/>
[2]: <https://reviews.llvm.org/rG670944fb20b226fc22fa993ab521125f9adbd30a>
[3]: <https://github.com/WebAssembly/wasi-sdk/issues/185>
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This is required to actually _build_ the Wasm language module.
The nxt_wasm_app_conf_t structure consists of the modules name, e.g
wasm, then the three required function handlers followed by the five
optional function handlers.
See the next commit for details of these function handlers.
We also need to include the u.wasm union entry that provides access to
the above structure.
The bulk of the configuration infrastructure will be added in a
subsequent commit.
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This is the first patch in adding WebAssembly language module support.
This just adds a new NXT_APP_WASM type, required by subsequent commits.
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This makes it much more clear what's what.
This is in preparation for adding WebAssembly language module support.
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
There are a couple of reports on GitHub about issues accessing Python
ASGI based applications over IPv6.
A request over IPv6 would result in an error like
2023/05/13 17:49:12 [alert] 47202#47202 [unit] #10: Python failed to create 'client' pair
2023/05/13 17:49:12 [alert] 47202#47202 [unit] Python failed to call 'loop.call_soon'
ValueError: invalid literal for int() with base 10: 'db8:1:1:1ee7:dead:beef:cafe'
The above error was the direct cause of the following exception:
Traceback (most recent call last):
File "/usr/lib64/python3.11/asyncio/base_events.py", line 765, in call_soon
handle = self._call_soon(callback, args, context)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib64/python3.11/asyncio/base_events.py", line 781, in _call_soon
handle = events.Handle(callback, args, self, context)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
SystemError: <class 'asyncio.events.Handle'> returned a result with an exception set
This issue occurred in the nxt_py_asgi_create_ip_address() function
where it tries to create an IP address / port number pair.
It does this by looking for the first ':' in the address and taking
everything after it as the port number. Like in the above error message,
if we tried to access the server @ 2001:db8:1:1:1ee7:dead:beef:cafe,
then we'd end up with the port number as 'db8:1:1:1ee7:dead:beef:cafe'.
There are two issues with this
1) The IP address and port number are already flowed through
separately.
2) Even if (1) wasn't true, it would still be broken for IPv6 as we'd
expect to a get an address literal like
[2001:db8:1:1:1ee7:dead:beef:cafe]:8080, however there was no code to
handle the []'s.
The fix is to simply not try looking for a port number. We pass a port
number into this function to use in the case where we don't find a port
number, we never will...
A further cleanup would be to flow through the server port number when
creating the 'server pair' PyTuple, rather than just using the hard
coded 80.
Closes: <https://github.com/nginx/unit/issues/793>
Closes: <https://github.com/nginx/unit/issues/874>
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
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>
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>
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>
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>
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>
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>
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<>"
[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>
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>
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>
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>
When running as a daemon. unit currently sets umask(0), i.e no umask.
This is resulting in various directories being created with a mode of
0777, e.g
rwxrwxrwx
this is currently affecting cgroup and rootfs directories, which are
being created with a mode of 0777, and when running as a daemon as there
is no umask to restrict the permissions.
This also affects the language modules (the umask is inherited over
fork(2)) whereby unless something explicitly sets a umask, files and
directories will be created with full permissions, 0666 (rw-rw-rw-)/
0777 (rwxrwxrwx) respectively.
This could be an unwitting security issue.
My original idea was to just remove the umask(0) call and thus inherit
the umask from the executing shell/program.
However there was some concern about just inheriting whatever umask was
in effect.
Alex suggested that rather than simply removing the umask(0) call we
change it to a value of 022 (which is a common default), which will
result in directories and files with permissions at most of 0755
(rwxr-xr-x) & 0644 (rw-r--r--).
If applications need some other umask set, they can (as they always have
been able to) set their own umask(2).
Suggested-by: Alejandro Colomar <alx.manpages@gmail.com>
Reviewed-by: Liam Crilly <liam@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
When using the 'rootfs' isolation option, by default a tmpfs filesystem
is mounted on tmp/. Currently this is mounted with a mode of 0777, i.e
drwxrwxrwx. 3 root root 60 Feb 22 11:56 tmp
however this should really have the sticky bit[0] set (as is per-normal for
such directories) to prevent users from having free reign on the files
contained within.
What we really want is it mounted with a mode of 01777, i.e
drwxrwxrwt. 3 root root 60 Feb 22 11:57 tmp
[0]: To quote inode(7)
"The sticky bit (S_ISVTX) on a directory means that a file in that
directory can be renamed or deleted only by the owner of the file, by
the owner of the directory, and by a privileged process."
Reviewed-by: Liam Crilly <liam@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
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>
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>
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>
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)'
e9e5ddd5a5 Refactor of process management.
6f2c9acd18 Processes refactoring. The cycle has been renamed to the runtime.
$ git log --pretty=oneline -S'if (cycle->listen_sockets != NULL) {'
6f2c9acd18 Processes refactoring. The cycle has been renamed to the runtime.
16cbf3c076 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>
If we don't remove the Unix domain listen socket file then when Unit
restarts it get an error like
2023/02/25 23:10:11 [alert] 36388#36388 bind(\"unix:/tmp/unit.sock\") failed (98: Address already in use)
This patch makes use of the listen_sockets array, that is already
allocated in the main process but never populated, to place the Unix
domain listen sockets into.
At shutdown we can then loop through this array and unlink(2) any Unix
domain sockets found therein.
Closes: <https://github.com/nginx/unit/issues/792>
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
In nxt_router_prepare_msg() we create a buffer (nxt_unit_request_t *req)
that gets sent to an application process that contains details about a
client request.
This buffer was always a little larger than needed due to allocating space
for the remote address _and_ port and the local address _and_ port. We
also allocate space for the local port separately.
->{local,remote}->length includes the port number and ':' and also the
'[]' for IPv6. E.g [2001:db8::1]:8080
->{local,remote}->address_length represents the length of the unadorned
IP address. E.g 2001:db8::1
Update the buffer size so that we only allocate what is actually needed.
Suggested-by: Zhidao HONG <z.hong@f5.com>
Cc: Zhidao HONG <z.hong@f5.com>
Reviewed-by: Zhidao HONG <z.hong@f5.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
User @bes-internal reported a Perl module crasher on GitHub.
This was due to a Perl application sending back two responses, for each
response we would call down into XS_NGINX__Unit__Sandbox_cb(), the first
time pctx->req would point to a valid nxt_unit_request_info_t, the
second time pctx->req would be NULL.
Add an invalid responses check which covers this case.
Closes: <https://github.com/nginx/unit/issues/841>
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This fixes an issue reported by @Peter2121 on GitHub.
In nxt_router_prepare_msg() we create a buffer (nxt_unit_request_t *req)
that gets sent to an application process that contains details about a
client request.
The req structure comprises various members with the final member being
an array (specified as a flexible array member, with its actual length
denoted by the req->fields_count member) of nxt_unit_field_t's. These
structures specify the length and offset for the various request headers
name/value pairs which are stored after some request metadata that is
stored immediately after this array of structs as individual nul
terminated strings.
After this we have the body content data (if any). So it looks a little
like
(gdb) x /64bs 0x7f38c976e060
0x7f38c976e060: "\353\346\244\t\006" <-- First nxt_unit_field_t
0x7f38c976e066: ""
0x7f38c976e067: ""
0x7f38c976e068: "T\001"
0x7f38c976e06b: ""
0x7f38c976e06c: "Z\001"
0x7f38c976e06f: ""
...
0x7f38c976e170: "\362#\244\v$" <-- Last nxt_unit_field_t
0x7f38c976e176: ""
0x7f38c976e177: ""
0x7f38c976e178: "\342\002"
0x7f38c976e17b: ""
0x7f38c976e17c: "\352\002"
0x7f38c976e17f: ""
0x7f38c976e180: "POST" <-- Start of request metadata
0x7f38c976e185: "HTTP/1.1"
0x7f38c976e18e: "unix:"
0x7f38c976e194: "unix:/dev/shm/842.sock"
0x7f38c976e1ab: ""
0x7f38c976e1ac: "fedora"
0x7f38c976e1b3: "/842.php"
0x7f38c976e1bc: "HTTP_HOST" <-- Start of header fields
0x7f38c976e1c6: "fedora"
0x7f38c976e1cd: "HTTP_X_FORWARDED_PROTO"
0x7f38c976e1e4: "https"
...
0x7f38c976e45a: "HTTP_COOKIE"
0x7f38c976e466: "PHPSESSID=8apkg25r9s9vju3pi085i21eh4"
0x7f38c976e48b: "public_form=sended" <-- Body content
Well that's how things are supposed to look! When using Unix domain
sockets what we actually got looked like
...
0x7f6141f3445a: "HTTP_COOKIE"
0x7f6141f34466: "PHPSESSID=uo5b2nu9buijkc89jotbgmd60vpublic_form=sended"
Here, the body content (from a POST for example) has been appended
straight onto the end of the last header field value. In this case
corrupting the PHP session cookie. The body content would still be
found by the application as its offset into this buffer is correct.
This problem was actually caused by a0327445 ("PHP: allowed to specify
URLs without a trailing '/'.") which added an extra item into this
request buffer specifying the port number that unit is listening on that
handled this request.
Unfortunately when I wrote that patch I didn't increase the size of this
request buffer to accommodate it.
When using normal TCP sockets we actually end up allocating more space
than required for this buffer, we track the end of this buffer up to
where the body content would go and so we have a few spare bytes between
the nul byte of the last field header value and the start of the body
content.
When using Unix domain sockets, they have no associated port number and
thus the port number has a length of 0 bytes, but we still write a '\0'
in there using up a byte that we didn't account for, this causes us to
loose the nul byte of the last header fields value causing the body data
to be appended to the last header field value.
The fix is simple, account for the local port length, we also add 1 to
it, this covers the nul byte, even if there is no port as with Unix
domain sockets.
Closes: <https://github.com/nginx/unit/issues/842>
Fixes: a0327445 ("PHP: allowed to specify URLs without a trailing '/'.")
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
When running as a daemon. unit currently sets umask(0), i.e no umask.
This is resulting in various directories being created with a mode of
0777, e.g
rwxrwxrwx
this is currently affecting cgroup and rootfs directories, which are
being created with a mode of 0777, and when running as a daemon as there
is no umask to restrict the permissions.
This also affects the language modules (the umask is inherited over
fork(2)) whereby unless something explicitly sets a umask, files and
directories will be created with full permissions, 0666 (rw-rw-rw-)/
0777 (rwxrwxrwx) respectively.
This could be an unwitting security issue.
My original idea was to just remove the umask(0) call and thus inherit
the umask from the executing shell/program.
However there was some concern about just inheriting whatever umask was
in effect.
Alex suggested that rather than simply removing the umask(0) call we
change it to a value of 022 (which is a common default), which will
result in directories and files with permissions at most of 0755
(rwxr-xr-x) & 0644 (rw-r--r--).
If applications need some other umask set, they can (as they always have
been able to) set their own umask(2).
Suggested-by: Alejandro Colomar <alx.manpages@gmail.com>
Reviewed-by: Liam Crilly <liam@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
When using the 'rootfs' isolation option, by default a tmpfs filesystem
is mounted on tmp/. Currently this is mounted with a mode of 0777, i.e
drwxrwxrwx. 3 root root 60 Feb 22 11:56 tmp
however this should really have the sticky bit[0] set (as is per-normal for
such directories) to prevent users from having free reign on the files
contained within.
What we really want is it mounted with a mode of 01777, i.e
drwxrwxrwt. 3 root root 60 Feb 22 11:57 tmp
[0]: To quote inode(7)
"The sticky bit (S_ISVTX) on a directory means that a file in that
directory can be renamed or deleted only by the owner of the file, by
the owner of the directory, and by a privileged process."
Reviewed-by: Liam Crilly <liam@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Since the previous commit, nxt_getpid() is only ever aliased to
getpid(2).
nxt_getpid() was only used once in the code, while there are multiple
direct uses of getpid(2)
$ grep -r "getpid()" src/
src/nxt_unit.c: nxt_unit_pid = getpid();
src/nxt_process.c: nxt_pid = nxt_getpid();
src/nxt_process.c: nxt_pid = getpid();
src/nxt_lib.c: nxt_pid = getpid();
src/nxt_process.h:#define nxt_getpid() \
src/nxt_process.h:#define nxt_getpid() \
src/nxt_process.h: getpid()
Just remove it and convert the _single_ instance of nxt_getpid() to
getpid(2).
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
When using SYS_clone we used the getpid kernel system call directly via
syscall(SYS_getpid) to avoid issues with cached pids.
However since we are now only using fork(2) (+ unshare(2) for
namespaces) we no longer need to call the kernel getpid directly as the
fork(2) will ensure the cached pid is invalidated.
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
On GitHub, @razvanphp & @hbernaciak both reported issues running the
APCu PHP module under Unit.
When using this module they were seeing errors like
'apcu_fetch(): Failed to acquire read lock'
However when running APCu under php-fpm, everything was fine.
The issue turned out to be due to our use of SYS_clone breaking the
pthreads(7) API used by APCu. Even if we had been using glibc's
clone(2) wrapper we would still have run into problems due to a known
issue there.
Essentially the problem is when using clone, glibc doesn't update the
TID cache, so the child ends up having the same TID as the parent and
that is used in various parts of pthreads(7) such as in the various
locking primitives, so when APCu was grabbing a lock it ended up using
the TID of the main unit process (rather than that of the php
application processes that was grabbing the lock).
So due to the above what was happening was when one of the application
processes went to grab either a read or write lock, the lock was
actually being attributed to the main unit process. If a process had
acquired the write lock, then if a process tried to acquire a read or
write lock then glibc would return EDEADLK due to detecting a deadlock
situation due to thinking the process already held the write lock when
in fact it didn't.
It seems the right way to do this is via fork(2) and unshare(2). We
already use fork(2) on other platforms.
This requires a few tricks to keep the essence of the processes the same
as before when using clone
1) We use the prctl(2) PR_SET_CHILD_SUBREAPER option (if its
available, since Linux 3.4) to make the main unit process inherit
prototype processes after a double fork(2), rather than them being
reparented to 'init'.
This avoids needing to ^C twice to fully exit unit when running in
the foreground. It's probably also better if they maintain their
parent child relationship where possible.
2) We use a double fork(2) technique on the prototype processes to
ensure they themselves end up in a new PID namespace as PID 1 (when
CLONE_NEWPID is being used).
When using unshare(CLONE_NEWPID), the calling process is _not_
placed in the namespace (as discussed in pid_namespaces(7)). It
only sets things up so that subsequent children are placed in a PID
namespace.
Having the prototype processes as PID 1 in the new PID namespace is
probably a good thing and matches the behaviour of clone(2). Also,
some isolation tests break if the prototype process is not PID 1.
3) Due to the above double fork(2) the main unit process looses track
of the prototype process ID, which it needs to know.
To solve this, we employ a simple pipe(2) between the main unit and
prototype processes and pass the prototype grandchild PID from the
parent of the second fork(2) before exiting. This needs to be done
from the parent and not the grandchild, as the grandchild will see
itself having a PID of 1 while the main process needs its
externally visible PID.
Link: <https://www.php.net/manual/en/book.apcu.php>
Link: <https://sourceware.org/bugzilla/show_bug.cgi?id=21793>
Closes: <https://github.com/nginx/unit/issues/694>
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This prctl(2) option can be used to set the "child subreaper" attribute
of the calling process. This allows a process to take on the role of
'init', which means the process will inherit descendant processes when
their immediate parent terminates.
This will be used in an upcoming commit that uses a double fork(2) +
unshare(2) to create a new PID namespace. The parent from the second
fork will terminate leaving the child process to be inherited by 'init'.
Aside from it being better to maintain the parent/child relationships
between the various unit processes, without setting this you need to ^C
twice to fully quit unit when running in the foreground after the double
fork.
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Due to the need to replace our use of clone/__NR_clone on Linux with
fork(2)/unshare(2) for enabling Linux namespaces(7) to keep the
pthreads(7) API working. Let's rename NXT_HAVE_CLONE to
NXT_HAVE_LINUX_NS, i.e name it after the feature, not how it's
implemented, then in future if we change how we do namespaces again we
don't have to rename this.
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This prctl(2) option is checked for in auto/isolation, unfortunately due
to a typo this feature has never been enabled.
In the auto/isolation script the feature name was down as
NXT_HAVE_PR_SET_NO_NEW_PRIVS0, which means we end up with the following
in build/nxt_auto_config.h
#ifndef NXT_HAVE_PR_SET_NO_NEW_PRIVS0
#define NXT_HAVE_PR_SET_NO_NEW_PRIVS0 1
#endif
Whereas everywhere else is checking for NXT_HAVE_PR_SET_NO_NEW_PRIVS.
This also guards the inclusion of sys/prctl.h in src/nxt_process.c which
is required by a subsequent commit.
Fixes: e2b53e1 ("Added "rootfs" feature.")
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This adds a check to nxt_python_asgi_get_event_loop() on the
event_loop_func name in the case that running that function fails, and
if it's get_running_loop() that failed we skip printing an error message
as this is an often expected behaviour since the previous commit and we
don't want users reporting erroneous bugs.
This check will always happen regardless of Python version while it
really only applies to Python >= 3.7, there didn't seem much point
adding complexity to the code for this case and in what will be an ever
diminishing case of people running older Pythons.
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Several users on GitHub reported issues with running Python ASGI apps on
Unit with Python 3.11.1 (this would also effect Python 3.10.9) with the
following error from Unit
2023/01/15 22:43:22 [alert] 0#77128 [unit] Python failed to call 'asyncio.get_event_loop'
TL;DR
asyncio.get_event_loop() is currently broken due to the process of
deprecating part or all of it.
First some history.
In Unit we had this commit
commit 8dcb0b9987
Author: Max Romanov <max.romanov@nginx.com>
Date: Thu Nov 5 00:04:59 2020 +0300
Python: request processing in multiple threads.
One of things this did was to create a new asyncio event loop in each
thread using asyncio.new_event_loop().
It's perhaps worth noting that all these asyncio.* functions are Python
functions that we call from the C code in Unit.
Then we had this commit
commit f27fbd9b4d
Author: Max Romanov <max.romanov@nginx.com>
Date: Tue Jul 20 10:37:54 2021 +0300
Python: using default event_loop for main thread for ASGI.
This changed things so that Unit calls asyncio.get_event_loop() in the
_main_ thread (but still calls asyncio.new_event_loop() in the other
threads).
asyncio.get_event_loop() up until recently would either return an
already running event loop or return a newly created one.
This was done for $reasons that the commit message and GitHub issue #560
hint at. But the intimation is that there can already be an event loop
running from the application (I assume it's referring to the users
application) at this point and if there is we should use it.
Now for the Python side of things.
On the main branch we had
commit 172c0f2752d8708b6dda7b42e6c5a3519420a4e8
Author: Serhiy Storchaka <storchaka@gmail.com>
Date: Sun Apr 25 13:40:44 2021 +0300
bpo-39529: Deprecate creating new event loop in asyncio.get_event_loop() (GH-23554)
This commit began the deprecating of asyncio.get_event_loop().
commit fd38a2f0ec03b4eec5e3cfd41241d198b1ee555a
Author: Serhiy Storchaka <storchaka@gmail.com>
Date: Tue Dec 6 19:42:12 2022 +0200
gh-93453: No longer create an event loop in get_event_loop() (#98440)
This turned asyncio.get_event_loop() into a RuntimeError _if_ there
isn't a current event loop.
commit e5bd5ad70d9e549eeb80aadb4f3ccb0f2f23266d
Author: Serhiy Storchaka <storchaka@gmail.com>
Date: Fri Jan 13 14:40:29 2023 +0200
gh-100160: Restore and deprecate implicit creation of an event loop (GH-100410)
This re-creates the event loop if there wasn't one and emits a
deprecation warning.
After at least the last two commits Unit no longer works with the Python
_main_ branch.
Meanwhile on the 3.11 branch we had
commit 3fae04b10e2655a20a3aadb5e0d63e87206d0c67
Author: Serhiy Storchaka <storchaka@gmail.com>
Date: Tue Dec 6 17:15:44 2022 +0200
[3.11] gh-93453: Only emit deprecation warning in asyncio.get_event_loop when a new event loop is created (#99949)
which is what caused our breakage, though perhaps unintentionally as we
get the following traceback
Traceback (most recent call last):
File "/usr/lib64/python3.11/asyncio/events.py", line 676, in get_event_loop
f = sys._getframe(1)
^^^^^^^^^^^^^^^^
ValueError: call stack is not deep enough
2023/01/18 02:46:10 [alert] 0#180279 [unit] Python failed to call 'asyncio.get_event_loop'
However, regardless, it is clear we need to stop using
asyncio.get_event_loop().
One option is to switch to the higher level asyncio.run() API, however
that is a rather large change.
This commit takes the simpler approach of using
asyncio.get_running_loop() (which it seems get_event_loop() will
eventually be an alias of) in the _main_ thread to return the currently
running event loop, or if there is no current event loop, it will call
asyncio.new_event_loop() to return a newly created event loop.
I believe this mimics the current behaviour. In my testing
get_event_loop() seemed to always return a newly created loop, as when
just calling get_running_loop() it would return NULL and we would fail
out.
When running two processes each with 2 threads we would get the
following loops with Python 3.11.0 and unpatched Unit
<_UnixSelectorEventLoop running=False closed=False debug=False>
<_UnixSelectorEventLoop running=False closed=False debug=False>
<_UnixSelectorEventLoop running=False closed=False debug=False>
<_UnixSelectorEventLoop running=False closed=False debug=False>
and with Python 3.11.1 and a patched Unit we would get
<_UnixSelectorEventLoop running=False closed=False debug=False>
<_UnixSelectorEventLoop running=False closed=False debug=False>
<_UnixSelectorEventLoop running=False closed=False debug=False>
<_UnixSelectorEventLoop running=False closed=False debug=False>
Tested-by: Rafał Safin <rafal.safin12@gmail.com>
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This is a preparatory patch that factors out the asyncio event loop
creation code from nxt_python_asgi_ctx_data_alloc() into its own
function, to facilitate being called multiple times.
This a part of the work to move away from using the
asyncio.get_event_loop() function due to it no longer creating event
loops if there wasn't one running.
See the following commit for the gory details.
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Since the previous commit, we now properly handle 403 Forbidden & 404
Not Found errors in the PHP language module.
This adds a test for 403 Forbidden to test/test_php_application.py, but
also fixes a test in test/test_php_targets.py where we were checking for
503 but should have been a 404, which we now do.
Acked-by: Alejandro Colomar <alx@nginx.com>
Cc: Andrei Zeliankou <zelenkov@nginx.com>
[ Incorporates a couple of small test cleanups from Andrei ]
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Previously the PHP module would produce one of four status codes
200 OK
301 Moved Permanently
500 Internal Server Error
503 Service Unavailable
200 for successful requests, 301 for cases where the url was a directory
without a trailing '/', 500 for bad PHP or non-existing PHP file and 503
for all other errors.
With this commit we now handle missing files and directories, returning
404 Not Found and files and directories that don't allow access,
returning 403 Forbidden.
We do these checks in two places, when we check if we should do a
directory redirect (bar -> bar/) and in the nxt_php_execute() function.
One snag with the latter is that the php_execute_script() function only
returns success/failure (no reason). However while it took a
zend_file_handle structure with the filename of the script to run, we
can instead pass through an already opened file-pointer (FILE *) via
that structure. So we can try opening the script ourselves and do the
required checks before calling php_execute_script().
We also make use of the zend_stream_init_fp() function that initialises
the zend_file_handle structure if it's available otherwise we use our
own version. This is good because the zend_file_handle structure has
changed over time and the zend_stream_init_fp() function should change
with it.
Closes: <https://github.com/nginx/unit/issues/767>
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Cc: Andrei Zeliankou <zelenkov@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Create a const char *filename variable to hold
ctx->script_filename.start, which is a much more manageable name and
will negate the need for any more casting in the following commit when
we switch to using a FILE * instead of a filename in
php_execute_script().
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Cc: Andrei Zeliankou <zelenkov@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Where possible make use of the zend_stream_init_filename() function
introduced in PHP 7.4.
This is essentially a preparatory patch for switching to using an
already opened file-pointer in nxt_php_execute(). While wrapping this
new code in a PHP version check with a fallback to our own function is
perhaps slightly overkill, it does reduce the diff of the commit that
switches to a FILE *.
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Cc: Andrei Zeliankou <zelenkov@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
@dward on GitHub reported an issue with a URL like
http://foo.bar/test.php?blah=test.php/foo
where we would end up trying to run the script
test.php?blah=test.php
In the PHP module the format 'file.php/' is treated as a special case in
nxt_php_dynamic_request() where we check the _path_ part of the url for
the string '.php/'.
The problem is that the path actually also contains the query string,
thus we were finding 'test.php/' in the above URL and treating that
whole path as the script to run.
The fix is simple, replace the strstr(3) with a memmem(3), where we can
limit the amount of path we use for the check.
The trick here and what is not obvious from the code is that while
path.start points to the whole path including the query string,
path.length only contains the length of the _path_ part.
NOTE: memmem(3) is a GNU extension and is neither specified by POSIX or
ISO C, however it is available on a number of other systems, including:
FreeBSD, OpenBSD, NetBSD, illumos, and macOS.
If it comes to it we can implement a simple alternative for systems
which lack memmem(3).
This also adds a test case (provided by @dward) to cover this.
Closes: <https://github.com/nginx/unit/issues/781>
Cc: Andrei Zeliankou <zelenkov@nginx.com>
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Reviewed-by: Andrei Zeliankou <zelenkov@nginx.com> [test]
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
The nxt_websocket_header_t structure defines the layout of a websocket
frame header. As the websocket frame is mapped directly onto this
structure its layout needs to match how it's coming off the network.
The network being big endian means on big endian systems the structure
layout can simply match that of the websocket frame header. On little
endian systems we need to reverse the two bytes.
This was done via the BYTE_ORDER, BIG_ENDIAN and LITTLE_ENDIAN macros,
however these are not universal, e.g they are _not_ defined on illumos
(OpenSolaris / OpenIndiana) and so we get the following compiler errors
In file included from src/nxt_h1proto.c:12:0:
src/nxt_websocket_header.h:25:13: error: duplicate member 'opcode'
uint8_t opcode:4;
^~~~~~
src/nxt_websocket_header.h:26:13: error: duplicate member 'rsv3'
uint8_t rsv3:1;
^~~~
src/nxt_websocket_header.h:27:13: error: duplicate member 'rsv2'
uint8_t rsv2:1;
^~~~
src/nxt_websocket_header.h:28:13: error: duplicate member 'rsv1'
uint8_t rsv1:1;
^~~~
src/nxt_websocket_header.h:29:13: error: duplicate member 'fin'
uint8_t fin:1;
^~~
src/nxt_websocket_header.h:31:13: error: duplicate member 'payload_len'
uint8_t payload_len:7;
^~~~~~~~~~~
src/nxt_websocket_header.h:32:13: error: duplicate member 'mask'
uint8_t mask:1;
^~~~
This commit fixes that by using the new NXT_HAVE_{BIG,LITTLE}_ENDIAN
macros introduced in the previous commit.
Closes: <https://github.com/nginx/unit/issues/297>
Fixes: e501c74 ("Introducing websocket support in router and libunit.")
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
In configure we set NXT_HAVE_LITTLE_ENDIAN for i386, amd64 and x86_64.
However that misses at least AArch64 (arm64) where it's usually run in
little endian mode.
However none of that really matters as NXT_HAVE_LITTLE_ENDIAN isn't used
anywhere. So why this patch?
The only place we need to explicitly know about endianness is the
nxt_websocket_header_t structure where we lay it out differently
depending on endianness.
This is currently done using BYTE_ORDER, LITTLE_ENDIAN and BIG_ENDIAN
macros.
However on at least illumos (OpenSolaris / OpenIndiana) those macros are
not defined and we get compiler errors due to duplicate structure
members.
So let's use our own NXT_HAVE_{BIG,LITTLE}_ENDIAN macros. However it
would be better to detect endianness programmatically as some
architectures can run in either mode, e.g Linux used to run in big
endian on PowerPC but has since switched to little endian (to match
x86).
This commit adds an auto/endian script (using a slightly modified
version of the test program from nginx's auto script), that checks for
the endianness of the platform being built on. E.g
checking for endianness ... little endian
The next commit will switch the nxt_websocket_header_t structure over to
these new macros.
Link: <https://github.com/nginx/unit/pull/298>
Link: <https://developer.ibm.com/articles/l-power-little-endian-faq-trs/>
Tested-by: Alejandro Colomar <alx@nginx.com>
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
There was a couple of reports of Python applications failing due to the
following type of error
File "/opt/netbox/netbox/netbox/configuration.py", line 25, in _import
print(f"\U0001f9ec loaded config '{path}'")
UnicodeEncodeError: 'ascii' codec can't encode character '\U0001f9ec' in
position 0: ordinal not in range(128)
due to the use of Unicode text in the print() statement.
This only happened for python 3.8+ when using the "home" configuration
option as this meant we were going through the new PyConfig
configuration.
When using this new configuration method with the 'isolated' specific
API (for embedded Python) UTF-8 is disabled by default,
PyPreConfig->utf8_mode = 0.
To fix this we need to setup the Python pre config and enable utf-8
mode. However rather than enable utf-8 unconditionally we can set to it
to -1 so that it will use the LC_CTYPE environment variable to determine
whether to enable utf-8 mode or not. utf-8 mode will be enabled if
LC_CTYPE is either: C, POSIX or some specific UTF-8 locale. This is the
default utf8_mode setting when using the non-isolated PyPreConfig API.
Reported-by: Tobias Genannt <tobias.genannt@kappa-velorum.net>
Tested-by: Tobias Genannt <tobias.genannt@kappa-velorum.net>
Link: <https://peps.python.org/pep-0587/>
Link: <https://docs.python.org/3/c-api/init_config.html#c.PyPreConfig.utf8_mode>
Fixes: 491d0f70 ("Python: Added support for Python 3.11.")
Closes: <https://github.com/nginx/unit/issues/817>
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This is a preparatory patch for future work and cleans up the code a
little in the Python 3.8+ variant of nxt_python3_init_config().
The main advantage being we no longer have calls to PyConfig_Clear() in
two different paths.
The variables have a little extra space in their declarations to allow
for the next patch which introduces a variable with a longer type name,
which will help reduce the size of the diff.
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Since the previous commit, nxt_getpid() is only ever aliased to
getpid(2).
nxt_getpid() was only used once in the code, while there are multiple
direct uses of getpid(2)
$ grep -r "getpid()" src/
src/nxt_unit.c: nxt_unit_pid = getpid();
src/nxt_process.c: nxt_pid = nxt_getpid();
src/nxt_process.c: nxt_pid = getpid();
src/nxt_lib.c: nxt_pid = getpid();
src/nxt_process.h:#define nxt_getpid() \
src/nxt_process.h:#define nxt_getpid() \
src/nxt_process.h: getpid()
Just remove it and convert the _single_ instance of nxt_getpid() to
getpid(2).
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This is an extension to the previous commit, which made
large_header_buffer_size a valid configuration setting.
This commit makes a related value, large_header_buffers, a valid
configuration setting.
While large_header_buffer_size effectively limits the maximum size of
any single header (although unit will try to pack multiple headers into
a buffer if they wholly fit).
large_header_buffers limits how many of these 'large' buffers are
available. It makes sense to also allow this to be user set.
large_header_buffers is already set by the configuration system in
nxt_router.c it just isn't set as a valid config option in
nxt_conf_validation.c
With this change users can set this option in their config if required
by the following
"settings": {
"http": {
"large_header_buffers": 8
}
},
It retains its default value of 4 if this is not set.
NOTE: This is being released as undocumented and subject to change as it
exposes internal workings of unit.
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
@JanMikes and @tagur87 on GitHub both reported issues with long URLs
that were exceeding the 8192 byte large_header_buffer_size setting,
which resulted in a HTTP 431 error (Request Header Fields Too Large).
This can be resolved in the code by updating the following line in
src/nxt_router.c::nxt_router_conf_create()
skcf->large_header_buffer_size = 8192;
However, requiring users to modify unit and install custom versions is
less than ideal. We could increase the value, but to what?
This commit takes the option of allowing the user to set this option in
their config by making large_header_buffer_size a valid configuration
setting.
large_header_buffer_size is already set by the configuration system in
nxt_router.c it just isn't set as a valid config option in
nxt_conf_validation.c
With this change users can set this option in their config if required
by the following
"settings": {
"http": {
"large_header_buffer_size": 16384
}
},
It retains its default value of 8192 bytes if this is not set.
With this commit, without the above setting or too low a value, with a
long URL you get a 431 error. With the above setting set to a large
enough value, the request is successful.
NOTE: This setting really determines the maximum size of any single
header _value_. Also, unit will try and place multiple values
into a buffer _if_ they fully fit.
NOTE: This is being released as undocumented and subject to change as it
exposes internal workings of unit.
Closes: <https://github.com/nginx/unit/issues/521>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This hooks the cgroup support up to the config system so it can actually
be used.
To make use of this in unit a new "cgroup" section has been added to the
isolation configuration.
e.g
"applications": {
"python": {
"type": "python",
"processes": 5,
"path": "/opt/unit/unit-cgroup-test/",
"module": "app",
"isolation": {
"cgroup": {
"path": "app/python"
}
}
}
}
Now there are two ways to specify the path, relative, like the above
(without a leading '/') and absolute (with a leading '/').
In the above case the "python" application is placed into its own cgroup
under CGROUP_ROOT/<main unit process cgroup>/app/python. Whereas if you
specified say
"path": "/unit/app/python"
Then the python application would be placed under
CGROUP_ROOT/unit/app/python
The first option allows you to easily take advantage of any resource
limits that have already been configured for unit.
With the second method (absolute pathname) if you know of an already
existing cgroup where you'd like to place it, you can, e.g
"path": "/system.slice/unit/python"
Where system.slice has already been created by systemd and may already
have some overall system limits applied which would also apply to unit.
Limits apply down the hierarchy and lower groups can't exceed the
previous group limits.
So what does this actually look like? Lets take the unit-calculator
application[0] and have each of its applications placed into their own
cgroup. If we give each application a new section like
"isolation": {
"cgroup": {
"path": "/unit/unit-calculator/add"
}
}
changing the path for each one, we can visualise the result with the
systemd-cgls command, e.g
│ └─session-5.scope (#4561)
│ ├─ 6667 sshd: andrew [priv]
│ ├─ 6684 sshd: andrew@pts/0
│ ├─ 6685 -bash
│ ├─ 12632 unit: main v1.28.0 [/opt/unit/sbin/unitd --control 127.0.0.1:808>
│ ├─ 12634 unit: controller
│ ├─ 12635 unit: router
│ ├─ 13550 systemd-cgls
│ └─ 13551 less
├─unit (#4759)
│ └─unit-calculator (#5037)
│ ├─subtract (#5069)
│ │ ├─ 12650 unit: "subtract" prototype
│ │ └─ 12651 unit: "subtract" application
│ ├─multiply (#5085)
│ │ ├─ 12653 unit: "multiply" prototype
│ │ └─ 12654 unit: "multiply" application
│ ├─divide (#5101)
│ │ ├─ 12671 unit: "divide" prototype
│ │ └─ 12672 node divide.js
│ ├─sqroot (#5117)
│ │ ├─ 12679 unit: "sqroot" prototype
│ │ └─ 12680 /home/andrew/src/unit-calculator/sqroot/sqroot
│ └─add (#5053)
│ ├─ 12648 unit: "add" prototype
│ └─ 12649 unit: "add" application
We used an absolute path so the cgroups will be created relative to the
main cgroupfs mount, e.g /sys/fs/cgroup
We can see that the main unit processes are in the same cgroup as the
shell from where they were started, by default child process are placed
into the same cgroup as the parent.
Then we can see that each application has been placed into its own
cgroup under /sys/fs/cgroup
Taking another example of a simple 5 process python application, with
"isolation": {
"cgroup": {
"path": "app/python"
}
}
Here we have specified a relative path and thus the python application
will be placed below the existing cgroup that contains the main unit
process. E.g
│ │ │ ├─app-glib-cinnamon\x2dcustom\x2dlauncher\x2d3-43951.scope (#90951)
│ │ │ │ ├─ 988 unit: main v1.28.0 [/opt/unit/sbin/unitd --no-daemon]
│ │ │ │ ├─ 990 unit: controller
│ │ │ │ ├─ 991 unit: router
│ │ │ │ ├─ 43951 xterm -bg rgb:20/20/20 -fg white -fa DejaVu Sans Mono
│ │ │ │ ├─ 43956 bash
│ │ │ │ ├─ 58828 sudo -i
│ │ │ │ ├─ 58831 -bash
│ │ │ │ └─app (#107351)
│ │ │ │ └─python (#107367)
│ │ │ │ ├─ 992 unit: "python" prototype
│ │ │ │ ├─ 993 unit: "python" application
│ │ │ │ ├─ 994 unit: "python" application
│ │ │ │ ├─ 995 unit: "python" application
│ │ │ │ ├─ 996 unit: "python" application
│ │ │ │ └─ 997 unit: "python" application
[0]: <https://github.com/lcrilly/unit-calculator>
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This commit enables the building of the cgroup code. This is only built
when the cgroupv2 filesystem is found.
If cgroupv2 support is found then
cgroupv2: .................. YES
will be printed by ./configure
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This commit hooks into the cgroup infrastructure added in the previous
commit to create per-application cgroups.
It does this by adding each "prototype process" into its own cgroup,
then each child process inherits its parents cgroup.
If we fail to create a cgroup we simply fail the process. This behaviour
may get enhanced in the future.
This won't actually do anything yet. Subsequent commits will hook this
up to the build and config systems.
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Firstly, this is not to be confused with CLONE_NEWCGROUP which unit
already supports and is related to namespaces. To re-cap, namespaces
allow processes to have different views of various parts of the system
such as filesystem mounts, networking, hostname etc.
Whereas cgroup[0] is a Linux kernel facility for collecting a bunch of
processes together to perform some task on the group as a whole, for
example to implement resource limits.
There are two parts to cgroup, the core part of organising processes
into a hierarchy and the controllers which are responsible for enforcing
resource limits etc.
There are currently two versions of the cgroup sub-system, the original
cgroup and a version 2[1] introduced in 3.16 (August 2014) and marked
stable in 4.5 (March 2016).
This commit supports the cgroup V2 API and implements the ability to
place applications into their own cgroup on a per-application basis.
You can put them each into their own cgroup or you can group some
together. The ability to set resource limits can easily be added in
future.
The initial use case of this would be to aid in observability of unit
applications which becomes much easier if you can just monitor them on a
per cgroup basis.
One thing to note about cgroup, is that unlike namespaces which are
controlled via system calls such as clone(2) and unshare(2), cgroups are
setup and controlled through the cgroupfs pseudo-filesystem.
cgroup is Linux only and this support will only be enabled if configure
finds the cgroup2 filesystem mount, e.g
cgroup2 on /sys/fs/cgroup type cgroup2 (rw,nosuid,nodev,noexec,relatime,seclabel,nsdelegate,memory_recursiveprot)
The cgroups are removed on shutdown or as required on reconfiguration.
This commit just adds the basic infrastructure for using cgroups within
unit. Subsequent commits will wire up this support.
It supports creating cgroups relative to the main cgroup root and also
below the cgroup of the main unit process.
[0]: <https://man7.org/linux/man-pages/man7/cgroups.7.html>
[1]: <https://www.kernel.org/doc/html/latest/admin-guide/cgroup-v2.html>
Cc: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Add simple wrapper functions for fopen(3) and fclose(3) that are
somewhat akin to the nxt_file_open() and nxt_file_close() wrappers that
log errors.
Suggested-by: Alejandro Colomar <alx@nginx.com>
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Andrei reported an issue with building unit when using '-O0' with GCC
producing the following compiler errors
cc -c -pipe -fPIC -fvisibility=hidden -O -W -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wmissing-prototypes -Werror -g -O0 -I src -I build \
\
\
-o build/src/nxt_unit.o \
-MMD -MF build/src/nxt_unit.dep -MT build/src/nxt_unit.o \
src/nxt_unit.c
src/nxt_unit.c: In function ‘nxt_unit_log’:
src/nxt_unit.c:6601:9: error: ‘msg’ may be used uninitialized [-Werror=maybe-uninitialized]
6601 | p = nxt_unit_snprint_prefix(p, end, pid, level);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/nxt_unit.c:6682:1: note: by argument 2 of type ‘const char *’ to ‘nxt_unit_snprint_prefix’ declared here
6682 | nxt_unit_snprint_prefix(char *p, const char *end, pid_t pid, int level)
| ^~~~~~~~~~~~~~~~~~~~~~~
src/nxt_unit.c:6582:22: note: ‘msg’ declared here
6582 | char msg[NXT_MAX_ERROR_STR], *p, *end;
| ^~~
src/nxt_unit.c: In function ‘nxt_unit_req_log’:
src/nxt_unit.c:6645:9: error: ‘msg’ may be used uninitialized [-Werror=maybe-uninitialized]
6645 | p = nxt_unit_snprint_prefix(p, end, pid, level);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/nxt_unit.c:6682:1: note: by argument 2 of type ‘const char *’ to ‘nxt_unit_snprint_prefix’ declared here
6682 | nxt_unit_snprint_prefix(char *p, const char *end, pid_t pid, int level)
| ^~~~~~~~~~~~~~~~~~~~~~~
src/nxt_unit.c:6625:35: note: ‘msg’ declared here
6625 | char msg[NXT_MAX_ERROR_STR], *p, *end;
| ^~~
cc1: all warnings being treated as errors
The above was reproduced with
$ ./configure --cc-opt=-O0 && ./configure python && make -j4
This warning doesn't happen on clang (15.0.4) or GCC (8.3) and seems to
have been introduced in GCC 11. The above is from GCC (12.2.1, Fedora
37).
The trigger of this GCC issue is actually part of a commit I introduced
a few months back to constify some function parameters and it seems the
consensus for how to resolve this problem is to simply remove the const
qualifier from the *end parameter to nxt_unit_snprint_prefix().
Reported-by: Andrei Zeliankou <zelenkov@nginx.com>
Link: <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100417>
Link: <https://github.com/samtools/htslib/pull/1285>
Link: <https://gcc.gnu.org/gcc-11/changes.html>
Fixes: 4418f99 ("Constified numerous function parameters.")
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
When using SYS_clone we used the getpid kernel system call directly via
syscall(SYS_getpid) to avoid issues with cached pids.
However since we are now only using fork(2) (+ unshare(2) for
namespaces) we no longer need to call the kernel getpid directly as the
fork(2) will ensure the cached pid is invalidated.
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
On GitHub, @razvanphp & @hbernaciak both reported issues running the
APCu PHP module under Unit.
When using this module they were seeing errors like
'apcu_fetch(): Failed to acquire read lock'
However when running APCu under php-fpm, everything was fine.
The issue turned out to be due to our use of SYS_clone breaking the
pthreads(7) API used by APCu. Even if we had been using glibc's
clone(2) wrapper we would still have run into problems due to a known
issue there.
Essentially the problem is when using clone, glibc doesn't update the
TID cache, so the child ends up having the same TID as the parent and
that is used in various parts of pthreads(7) such as in the various
locking primitives, so when APCu was grabbing a lock it ended up using
the TID of the main unit process (rather than that of the php
application processes that was grabbing the lock).
So due to the above what was happening was when one of the application
processes went to grab either a read or write lock, the lock was
actually being attributed to the main unit process. If a process had
acquired the write lock, then if a process tried to acquire a read or
write lock then glibc would return EDEADLK due to detecting a deadlock
situation due to thinking the process already held the write lock when
in fact it didn't.
It seems the right way to do this is via fork(2) and unshare(2). We
already use fork(2) on other platforms.
This requires a few tricks to keep the essence of the processes the same
as before when using clone
1) We use the prctl(2) PR_SET_CHILD_SUBREAPER option (if its
available, since Linux 3.4) to make the main unit process inherit
prototype processes after a double fork(2), rather than them being
reparented to 'init'.
This avoids needing to ^C twice to fully exit unit when running in
the foreground. It's probably also better if they maintain their
parent child relationship where possible.
2) We use a double fork(2) technique on the prototype processes to
ensure they themselves end up in a new PID namespace as PID 1 (when
CLONE_NEWPID is being used).
When using unshare(CLONE_NEWPID), the calling process is _not_
placed in the namespace (as discussed in pid_namespaces(7)). It
only sets things up so that subsequent children are placed in a PID
namespace.
Having the prototype processes as PID 1 in the new PID namespace is
probably a good thing and matches the behaviour of clone(2). Also,
some isolation tests break if the prototype process is not PID 1.
3) Due to the above double fork(2) the main unit process looses track
of the prototype process ID, which it needs to know.
To solve this, we employ a simple pipe(2) between the main unit and
prototype processes and pass the prototype grandchild PID from the
parent of the second fork(2) before exiting. This needs to be done
from the parent and not the grandchild, as the grandchild will see
itself having a PID of 1 while the main process needs its
externally visible PID.
Link: <https://www.php.net/manual/en/book.apcu.php>
Link: <https://sourceware.org/bugzilla/show_bug.cgi?id=21793>
Closes: <https://github.com/nginx/unit/issues/694>
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This prctl(2) option can be used to set the "child subreaper" attribute
of the calling process. This allows a process to take on the role of
'init', which means the process will inherit descendant processes when
their immediate parent terminates.
This will be used in an upcoming commit that uses a double fork(2) +
unshare(2) to create a new PID namespace. The parent from the second
fork will terminate leaving the child process to be inherited by 'init'.
Aside from it being better to maintain the parent/child relationships
between the various unit processes, without setting this you need to ^C
twice to fully quit unit when running in the foreground after the double
fork.
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Due to the need to replace our use of clone/__NR_clone on Linux with
fork(2)/unshare(2) for enabling Linux namespaces(7) to keep the
pthreads(7) API working. Let's rename NXT_HAVE_CLONE to
NXT_HAVE_LINUX_NS, i.e name it after the feature, not how it's
implemented, then in future if we change how we do namespaces again we
don't have to rename this.
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This prctl(2) option is checked for in auto/isolation, unfortunately due
to a typo this feature has never been enabled.
In the auto/isolation script the feature name was down as
NXT_HAVE_PR_SET_NO_NEW_PRIVS0, which means we end up with the following
in build/nxt_auto_config.h
#ifndef NXT_HAVE_PR_SET_NO_NEW_PRIVS0
#define NXT_HAVE_PR_SET_NO_NEW_PRIVS0 1
#endif
Whereas everywhere else is checking for NXT_HAVE_PR_SET_NO_NEW_PRIVS.
This also guards the inclusion of sys/prctl.h in src/nxt_process.c which
is required by a subsequent commit.
Fixes: e2b53e1 ("Added "rootfs" feature.")
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This adds a check to nxt_python_asgi_get_event_loop() on the
event_loop_func name in the case that running that function fails, and
if it's get_running_loop() that failed we skip printing an error message
as this is an often expected behaviour since the previous commit and we
don't want users reporting erroneous bugs.
This check will always happen regardless of Python version while it
really only applies to Python >= 3.7, there didn't seem much point
adding complexity to the code for this case and in what will be an ever
diminishing case of people running older Pythons.
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Several users on GitHub reported issues with running Python ASGI apps on
Unit with Python 3.11.1 (this would also effect Python 3.10.9) with the
following error from Unit
2023/01/15 22:43:22 [alert] 0#77128 [unit] Python failed to call 'asyncio.get_event_loop'
TL;DR
asyncio.get_event_loop() is currently broken due to the process of
deprecating part or all of it.
First some history.
In Unit we had this commit
commit 8dcb0b9987
Author: Max Romanov <max.romanov@nginx.com>
Date: Thu Nov 5 00:04:59 2020 +0300
Python: request processing in multiple threads.
One of things this did was to create a new asyncio event loop in each
thread using asyncio.new_event_loop().
It's perhaps worth noting that all these asyncio.* functions are Python
functions that we call from the C code in Unit.
Then we had this commit
commit f27fbd9b4d
Author: Max Romanov <max.romanov@nginx.com>
Date: Tue Jul 20 10:37:54 2021 +0300
Python: using default event_loop for main thread for ASGI.
This changed things so that Unit calls asyncio.get_event_loop() in the
_main_ thread (but still calls asyncio.new_event_loop() in the other
threads).
asyncio.get_event_loop() up until recently would either return an
already running event loop or return a newly created one.
This was done for $reasons that the commit message and GitHub issue #560
hint at. But the intimation is that there can already be an event loop
running from the application (I assume it's referring to the users
application) at this point and if there is we should use it.
Now for the Python side of things.
On the main branch we had
commit 172c0f2752d8708b6dda7b42e6c5a3519420a4e8
Author: Serhiy Storchaka <storchaka@gmail.com>
Date: Sun Apr 25 13:40:44 2021 +0300
bpo-39529: Deprecate creating new event loop in asyncio.get_event_loop() (GH-23554)
This commit began the deprecating of asyncio.get_event_loop().
commit fd38a2f0ec03b4eec5e3cfd41241d198b1ee555a
Author: Serhiy Storchaka <storchaka@gmail.com>
Date: Tue Dec 6 19:42:12 2022 +0200
gh-93453: No longer create an event loop in get_event_loop() (#98440)
This turned asyncio.get_event_loop() into a RuntimeError _if_ there
isn't a current event loop.
commit e5bd5ad70d9e549eeb80aadb4f3ccb0f2f23266d
Author: Serhiy Storchaka <storchaka@gmail.com>
Date: Fri Jan 13 14:40:29 2023 +0200
gh-100160: Restore and deprecate implicit creation of an event loop (GH-100410)
This re-creates the event loop if there wasn't one and emits a
deprecation warning.
After at least the last two commits Unit no longer works with the Python
_main_ branch.
Meanwhile on the 3.11 branch we had
commit 3fae04b10e2655a20a3aadb5e0d63e87206d0c67
Author: Serhiy Storchaka <storchaka@gmail.com>
Date: Tue Dec 6 17:15:44 2022 +0200
[3.11] gh-93453: Only emit deprecation warning in asyncio.get_event_loop when a new event loop is created (#99949)
which is what caused our breakage, though perhaps unintentionally as we
get the following traceback
Traceback (most recent call last):
File "/usr/lib64/python3.11/asyncio/events.py", line 676, in get_event_loop
f = sys._getframe(1)
^^^^^^^^^^^^^^^^
ValueError: call stack is not deep enough
2023/01/18 02:46:10 [alert] 0#180279 [unit] Python failed to call 'asyncio.get_event_loop'
However, regardless, it is clear we need to stop using
asyncio.get_event_loop().
One option is to switch to the higher level asyncio.run() API, however
that is a rather large change.
This commit takes the simpler approach of using
asyncio.get_running_loop() (which it seems get_event_loop() will
eventually be an alias of) in the _main_ thread to return the currently
running event loop, or if there is no current event loop, it will call
asyncio.new_event_loop() to return a newly created event loop.
I believe this mimics the current behaviour. In my testing
get_event_loop() seemed to always return a newly created loop, as when
just calling get_running_loop() it would return NULL and we would fail
out.
When running two processes each with 2 threads we would get the
following loops with Python 3.11.0 and unpatched Unit
<_UnixSelectorEventLoop running=False closed=False debug=False>
<_UnixSelectorEventLoop running=False closed=False debug=False>
<_UnixSelectorEventLoop running=False closed=False debug=False>
<_UnixSelectorEventLoop running=False closed=False debug=False>
and with Python 3.11.1 and a patched Unit we would get
<_UnixSelectorEventLoop running=False closed=False debug=False>
<_UnixSelectorEventLoop running=False closed=False debug=False>
<_UnixSelectorEventLoop running=False closed=False debug=False>
<_UnixSelectorEventLoop running=False closed=False debug=False>
Tested-by: Rafał Safin <rafal.safin12@gmail.com>
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This is a preparatory patch that factors out the asyncio event loop
creation code from nxt_python_asgi_ctx_data_alloc() into its own
function, to facilitate being called multiple times.
This a part of the work to move away from using the
asyncio.get_event_loop() function due to it no longer creating event
loops if there wasn't one running.
See the following commit for the gory details.
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Python 3.8 added a new Python initialisation configuration API[0].
Python 3.11 marked the old API as deprecated resulting in the following
compiler warnings which we treat as errors, failing the build
src/python/nxt_python.c: In function ‘nxt_python_start’:
src/python/nxt_python.c:130:13: error: ‘Py_SetProgramName’ is deprecated [-Werror=deprecated-declarations]
130 | Py_SetProgramName(nxt_py_home);
| ^~~~~~~~~~~~~~~~~
In file included from /opt/python-3.11/include/python3.11/Python.h:94,
from src/python/nxt_python.c:7:
/opt/python-3.11/include/python3.11/pylifecycle.h:37:38: note: declared here
37 | Py_DEPRECATED(3.11) PyAPI_FUNC(void) Py_SetProgramName(const wchar_t *);
| ^~~~~~~~~~~~~~~~~
src/python/nxt_python.c:134:13: error: ‘Py_SetPythonHome’ is deprecated [-Werror=deprecated-declarations]
134 | Py_SetPythonHome(nxt_py_home);
| ^~~~~~~~~~~~~~~~
/opt/python-3.11/include/python3.11/pylifecycle.h:40:38: note: declared here
40 | Py_DEPRECATED(3.11) PyAPI_FUNC(void) Py_SetPythonHome(const wchar_t *);
| ^~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
We actually have a few config scenarios: Python < 3, Python >= 3.0 < 3.8
and for Python 3 we have two configs where we select one based on
virtual environment setup.
Factor out the Python 3 config initialisation into its own function. We
actually create two functions, one for Python 3.8+ and one for older
Python 3. We pick the right function to use at build time.
The new API also has error checking (where the old API doesn't) which we
handle.
[0]: https://peps.python.org/pep-0587/
Closes: <https://github.com/nginx/unit/issues/710>
[ Andrew: Expanded upon patch from @sandeep-gh ]
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
@dward on GitHub reported an issue with a URL like
http://foo.bar/test.php?blah=test.php/foo
where we would end up trying to run the script
test.php?blah=test.php
In the PHP module the format 'file.php/' is treated as a special case in
nxt_php_dynamic_request() where we check the _path_ part of the url for
the string '.php/'.
The problem is that the path actually also contains the query string,
thus we were finding 'test.php/' in the above URL and treating that
whole path as the script to run.
The fix is simple, replace the strstr(3) with a memmem(3), where we can
limit the amount of path we use for the check.
The trick here and what is not obvious from the code is that while
path.start points to the whole path including the query string,
path.length only contains the length of the _path_ part.
NOTE: memmem(3) is a GNU extension and is neither specified by POSIX or
ISO C, however it is available on a number of other systems, including:
FreeBSD, OpenBSD, NetBSD, illumos, and macOS.
If it comes to it we can implement a simple alternative for systems
which lack memmem(3).
This also adds a test case (provided by @dward) to cover this.
Closes: <https://github.com/nginx/unit/issues/781>
Cc: Andrei Zeliankou <zelenkov@nginx.com>
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Reviewed-by: Andrei Zeliankou <zelenkov@nginx.com> [test]
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
The nxt_websocket_header_t structure defines the layout of a websocket
frame header. As the websocket frame is mapped directly onto this
structure its layout needs to match how it's coming off the network.
The network being big endian means on big endian systems the structure
layout can simply match that of the websocket frame header. On little
endian systems we need to reverse the two bytes.
This was done via the BYTE_ORDER, BIG_ENDIAN and LITTLE_ENDIAN macros,
however these are not universal, e.g they are _not_ defined on illumos
(OpenSolaris / OpenIndiana) and so we get the following compiler errors
In file included from src/nxt_h1proto.c:12:0:
src/nxt_websocket_header.h:25:13: error: duplicate member 'opcode'
uint8_t opcode:4;
^~~~~~
src/nxt_websocket_header.h:26:13: error: duplicate member 'rsv3'
uint8_t rsv3:1;
^~~~
src/nxt_websocket_header.h:27:13: error: duplicate member 'rsv2'
uint8_t rsv2:1;
^~~~
src/nxt_websocket_header.h:28:13: error: duplicate member 'rsv1'
uint8_t rsv1:1;
^~~~
src/nxt_websocket_header.h:29:13: error: duplicate member 'fin'
uint8_t fin:1;
^~~
src/nxt_websocket_header.h:31:13: error: duplicate member 'payload_len'
uint8_t payload_len:7;
^~~~~~~~~~~
src/nxt_websocket_header.h:32:13: error: duplicate member 'mask'
uint8_t mask:1;
^~~~
This commit fixes that by using the new NXT_HAVE_{BIG,LITTLE}_ENDIAN
macros introduced in the previous commit.
Closes: <https://github.com/nginx/unit/issues/297>
Fixes: e501c74 ("Introducing websocket support in router and libunit.")
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
In configure we set NXT_HAVE_LITTLE_ENDIAN for i386, amd64 and x86_64.
However that misses at least AArch64 (arm64) where it's usually run in
little endian mode.
However none of that really matters as NXT_HAVE_LITTLE_ENDIAN isn't used
anywhere. So why this patch?
The only place we need to explicitly know about endianness is the
nxt_websocket_header_t structure where we lay it out differently
depending on endianness.
This is currently done using BYTE_ORDER, LITTLE_ENDIAN and BIG_ENDIAN
macros.
However on at least illumos (OpenSolaris / OpenIndiana) those macros are
not defined and we get compiler errors due to duplicate structure
members.
So let's use our own NXT_HAVE_{BIG,LITTLE}_ENDIAN macros. However it
would be better to detect endianness programmatically as some
architectures can run in either mode, e.g Linux used to run in big
endian on PowerPC but has since switched to little endian (to match
x86).
This commit adds an auto/endian script (using a slightly modified
version of the test program from nginx's auto script), that checks for
the endianness of the platform being built on. E.g
checking for endianness ... little endian
The next commit will switch the nxt_websocket_header_t structure over to
these new macros.
Link: <https://github.com/nginx/unit/pull/298>
Link: <https://developer.ibm.com/articles/l-power-little-endian-faq-trs/>
Tested-by: Alejandro Colomar <alx@nginx.com>
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
There was a couple of reports of Python applications failing due to the
following type of error
File "/opt/netbox/netbox/netbox/configuration.py", line 25, in _import
print(f"\U0001f9ec loaded config '{path}'")
UnicodeEncodeError: 'ascii' codec can't encode character '\U0001f9ec' in
position 0: ordinal not in range(128)
due to the use of Unicode text in the print() statement.
This only happened for python 3.8+ when using the "home" configuration
option as this meant we were going through the new PyConfig
configuration.
When using this new configuration method with the 'isolated' specific
API (for embedded Python) UTF-8 is disabled by default,
PyPreConfig->utf8_mode = 0.
To fix this we need to setup the Python pre config and enable utf-8
mode. However rather than enable utf-8 unconditionally we can set to it
to -1 so that it will use the LC_CTYPE environment variable to determine
whether to enable utf-8 mode or not. utf-8 mode will be enabled if
LC_CTYPE is either: C, POSIX or some specific UTF-8 locale. This is the
default utf8_mode setting when using the non-isolated PyPreConfig API.
Reported-by: Tobias Genannt <tobias.genannt@kappa-velorum.net>
Tested-by: Tobias Genannt <tobias.genannt@kappa-velorum.net>
Link: <https://peps.python.org/pep-0587/>
Link: <https://docs.python.org/3/c-api/init_config.html#c.PyPreConfig.utf8_mode>
Fixes: 491d0f70 ("Python: Added support for Python 3.11.")
Closes: <https://github.com/nginx/unit/issues/817>
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This is a preparatory patch for future work and cleans up the code a
little in the Python 3.8+ variant of nxt_python3_init_config().
The main advantage being we no longer have calls to PyConfig_Clear() in
two different paths.
The variables have a little extra space in their declarations to allow
for the next patch which introduces a variable with a longer type name,
which will help reduce the size of the diff.
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Both @lucatacconi & @mwoodpatrick reported what appears to be the same
issue on GitHub. Namely that when using the PHP language module and
trying to access a URL that is a directory but without specifying the
trailing '/', they were getting a '503 Service Unavailable' error.
Note: This is when _not_ using the 'script' option.
E.g with the following config
{
"listeners": {
"[::1]:8080": {
"pass": "applications/php"
}
},
"applications": {
"php": {
"type": "php",
"root": "/var/tmp/unit-php"
}
}
}
and with a directory path of /var/tmp/unit-php/foo containing an
index.php, you would see the following
$ curl http://localhost/foo
<title>Error 503</title>
Error 503
However
$ curl http://localhost/foo/
would work and serve up the index.php
This commit fixes the above so you get the desired behaviour without
specifying the trailing '/' by doing the following
1] If the URL doesn't end in .php and doesn't have a trailing '/'
then check if the requested path is a directory.
2) If it is a directory then create a 301 re-direct pointing to it.
This matches the behaviour of the likes of nginx, Apache and
lighttpd.
This also matches the behaviour of the "share" action in Unit.
This doesn't effect the behaviour of the 'script' option which bypasses
the nxt_php_dynamic_request() function.
This also adds a couple of tests to test/test_php_application.py to
ensure this continues to work.
Closes: <https://github.com/nginx/unit/issues/717>
Closes: <https://github.com/nginx/unit/issues/753>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Future releases of GCC will render function definitions like
func()
invalid by default. See the previous commit 09f88c9 ("Fixed main()
prototypes in auto tests.") for details.
Such functions should be defined like
func(void)
This is a good thing to do regardless of the upcoming GCC changes.
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Future releases of GCC are planning to remove[0] default support for
some old features that were removed from C99 but GCC still accepts.
We can test for these changes by using the following -Werror=
directives
-Werror=implicit-int
-Werror=implicit-function-declaration
-Werror=int-conversion
-Werror=strict-prototypes
-Werror=old-style-definition
Doing so revealed an issue with the auto/ tests in that the test
programs always define main as
int main()
rather than
int main(void)
which results in a bunch of errors like
build/autotest.c:3:23: error: function declaration isn't a prototype [-Werror=strict-prototypes]
3 | int main() {
| ^~~~
build/autotest.c: In function 'main':
build/autotest.c:3:23: error: old-style function definition [-Werror=old-style-definition]
The fix was easy, it only required fixing the main prototype with
find -type f -exec sed -i 's/int main() {/int main(void) {/g' {} \;
Regardless of these upcoming GCC changes, this is probably a good thing
to do anyway for correctness.
[0]: https://fedoraproject.org/wiki/Changes/PortingToModernC
Link: <https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/message/CJXKTLXJUPZ4F2C2VQOTNMEA5JAUPMBD/>
Link: <https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/message/6SGHPHPAXKCVJ6PUZ57WVDQ5TDBVIRMF/>
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
@alejandro-colomar reported that the build was broken on MacOS
cc -o build/unitd -pipe -fPIC -fvisibility=hidden -O -W -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -fstrict-aliasing -Wstrict-overflow=5 -Wmissing-prototypes -Werror -g \
build/src/nxt_main.o build/libnxt.a \
\
\
-L/usr/local/Cellar/pcre2/10.40/lib -lpcre2-8
Undefined symbols for architecture x86_64:
"_nxt_fs_mkdir_parent", referenced from:
_nxt_runtime_pid_file_create in libnxt.a(nxt_runtime.o)
_nxt_runtime_controller_socket in libnxt.a(nxt_controller.o)
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [build/unitd] Error 1
This was due to commit 57fc920 ("Socket: Created control socket & pid file
directories.").
This happened because this commit introduced the usage of
nxt_fs_mkdir_parent() in core code which uses nxt_fs_mkdir(), both of
these are defined in src/nxt_fs.c. It turns out however that this file
doesn't get built on MacOS (or any system that isn't Linux or that
lacks a FreeBSD compatible nmount(2) system call) due to the following
In auto/sources we have
if [ $NXT_HAVE_ROOTFS = YES ]; then
NXT_LIB_SRCS="$NXT_LIB_SRCS src/nxt_fs.c"
fi
NXT_HAVE_ROOTFS is set in auto/isolation
If [ $NXT_HAVE_MOUNT = YES -a $NXT_HAVE_UNMOUNT = YES ]; then
NXT_HAVE_ROOTFS=YES
cat << END >> $NXT_AUTO_CONFIG_H
#ifndef NXT_HAVE_ISOLATION_ROOTFS
#define NXT_HAVE_ISOLATION_ROOTFS 1
#endif
END
fi
While we do have a check for a generic umount(2) which is found on
MacOS, for mount(2) we currently only check for the Linux mount(2) and
FreeBSD nmount(2) system calls. So NXT_HAVE_ROOTFS is set to NO on MacOS
and we don't build src/nxt_fs.c
This fixes the immediate build issue by taking the mount/umount OS
support out of nxt_fs.c into a new nxt_fs_mount.c file which is guarded
by the above while we now build nxt_fs.c unconditionally.
This should fix the build on any _supported_ system.
Reported-by: Alejandro Colomar <alx@nginx.com>
Fixes: 57fc920 ("Socket: Created control socket & pid file directories.")
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This is a preparatory patch that renames the 'local' and 'local_length'
members of the nxt_unit_request_t structure to 'local_addr' and
'local_addr_length' in preparation for the adding of 'local_port' and
'local_port_length' members.
Suggested-by: Zhidao HONG <z.hong@f5.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
@alejandro-colomar reported an issue on GitHub whereby Unit would fail
to start due to not being able to create the control socket (a Unix
Domain Socket)
2022/08/05 20:12:22 [alert] 21613#21613 bind(6,
unix:/opt/local/unit/var/run/unit/control.unit.sock.tmp)
failed (2: No such file or directory)
This could happen if the control socket was set to a directory that
doesn't exist. A common place to put the control socket would be under
/run/unit, and while /run will exist, /run/unit may well not (/run
is/should be cleared on each boot).
The pid file would also generally go under /run/unit, though this is
created after the control socket, however it could go someplace else so
we should also ensure its directory exists.
This commit will try to create the pid file and control sockets parent
directory. In some cases the user will need to ensure that the rest of
the path already exists.
This adds a new nxt_fs_mkdir_parent() function that given a full path
to a file (or directory), strips the last component off before passing
the remaining directory path to nxt_fs_mkdir().
Cc: Konstantin Pavlov <thresh@nginx.com>
Closes: <https://github.com/nginx/unit/issues/742>
Reported-by: Alejandro Colomar <alx@nginx.com>
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Tested-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This file is used by git (maybe also hg) to map different identities.
It can be used to map different names/email addresses to a preferred
name/email. So if you have authored/committed under different
names/email addresses you can map all these to a particular name/email.
Certain git commands (log, shortlog, show, blame etc) will consult this
file and show the Author/Committer accordingly.
Note: This does _not_ change history, this is simply used by various
commands to alter their output and can be disabled for some commands
with the --no-mailmap option.
This can be useful for commands like git shortlog so that all your
commits are shown under a single identity and also so people have an
up to date email address should they wish to contact you. And just for
overall consistency.
Seeing as I've already committed under two different email addresses
(not counting this one), I've put entries in this file to map my email
addresses to my @nginx.com one.
See also, gitmailmap(5).
Acked-by: Alex Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Andrew Clayton