Liam reported a problem when trying to restart wasm-wasi-component based
applications using the /control/applications/APPLICATION_NAME/restart
endpoint.
The application would become unresponsive.
What was happening was the old application process(es) weren't
exit(3)ing and so while we were starting new application processes, the
old ones were still hanging around in a non-functioning state.
When we are terminating an application it must call exit(3).
So that's what we do. We use the return value of nxt_unit_run() as the
exit status.
Due to exit(3)ing we also need to now explicitly handle the return on
error case.
Reported-by: Liam Crilly <liam@nginx.com>
Fixes: 20ada4b5c ("Wasm-wc: Core of initial Wasm component model language module support")
Closes: https://github.com/nginx/unit/issues/1179
Tested-by: Liam Crilly <liam@nginx.com>
Tested-by: Danielle De Leo <d.deleo@f5.com>
Co-developed-by: Dan Callahan <d.callahan@f5.com>
Signed-off-by: Dan Callahan <d.callahan@f5.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
cargo build creates the language module under
src/wasm-wasi-component/target/release/libwasm_wasi_component.so and not
build/lib/unit/modules/wasm_wasi_component.unit.so which is what we were
using as a target dependency in the Makefile which doesn't exist so this
resulted in the following
$ make wasm-wasi-component-install
cargo build --release --manifest-path src/wasm-wasi-component/Cargo.toml
Finished release [optimized] target(s) in 0.17s
install -d /opt/unit/modules
install -p src/wasm-wasi-component/target/release/libwasm_wasi_component.so \
/opt/unit/modules/wasm_wasi_component.unit.so
I.e it wanted to rebuild the module, after this patch we get the more
correct
$ make wasm-wasi-component-install
install -d /opt/unit/modules
install -p src/wasm-wasi-component/target/release/libwasm_wasi_component.so \
/opt/unit/modules/wasm_wasi_component.unit.so
This is all a little ugly because we're fighting against cargo wanting
to do its own thing and this wasm-wasi-component language module build
process is likely going to get some re-working anyway, so this will do
for now.
Reported-by: Konstantin Pavlov <thresh@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Rather than calling make itself to build nxt_unit.o make nxt_unit.o a
dependency of the main module build target.
Reported-by: Konstantin Pavlov <thresh@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This now includes support for the 'wasm-wasi-component' module.
This targets the upcoming 1.32.0 release which is required by
wasm-wasi-component. However of course the 1.32.0 tag doesn't exist yet,
so there will be a small window where this image won't build.
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
The minimum version required to build wasmtime 17 which is required by
wasm-wasi-component is 1.73.0
But no point not using the latest version.
This also now needs the libclang-dev package installed, we install this
via MODULE_PREBUILD_wasm.
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This exposes the various WebAssembly Component Model language module
specific options.
The application type is "wasm-wasi-component".
There is a "component" option that is required, this specifies the full
path to the WebAssembly component to be run. This component should be in
binary format, i.e a .wasm file.
There is also currently one optional option
"access"
Due to the sandboxed nature of WebAssembly, by default Wasm
modules/components don't have any access to the underlying filesystem.
There is however a capabilities based mechanism[0] for allowing such
access.
This adds a config option to the 'wasm-wasi-component' application type
(same as for 'wasm');
'access.filesystem' which takes an array of
directory paths that are then made available to the wasm
module/component. This access works recursively, i.e everything under a
specific path is allowed access to.
Example config might look like
"applications": {
"my-wasm-component": {
"type": "wasm-wasi-component",
"component": "/path/to/component.wasm",
"access" {
"filesystem": [
"/tmp",
"/var/tmp"
]
}
}
}
The actual mechanism used allows directories to be mapped differently in
the guest. But at the moment we don't support that and just map say /tmp
to /tmp. This can be revisited if it's something users clamour for.
[0]: <https://github.com/bytecodealliance/wasmtime/blob/main/docs/WASI-capabilities.md>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
It seems we do want to track this thing. This is just the latest version
that cargo had generated for me.
Cc: Dan Callahan <d.callahan@f5.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
With the initial port to wasmtime 17 we could no longer use the
'reactor' adaptor but had to switch to the more restrictive 'proxy'
adaptor.
This meant amongst other things (probably) we could no longer access the
filesystem.
Thanks to Joel Dice for pointing out the fix.
With this we can go back to using the 'reactor' adaptor again and things
are back to working as before.
It's worth noting that you can use either the 'proxy' or 'reactor'
adaptor depending on your requirements.
Cc: Joel Dice <joel.dice@fermyon.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
When Unit receives a request, if the body of that request is greater
than a certain amount (16KiB by default) then it is written to a
temporary file.
When a language module goes to read the request body in such situations
it will end up using read(2).
The wasm-wasi-component language module was failing to properly read
request bodies of around 2GiB or more.
This is because (on Linux at least) read(2) (and other related system
calls) will only read (or write) at most 0x7ffff000 (2,147,479,552)
bytes, this is the case for both 32 and 64-bit systems.
Regardless, it's probably not a good idea doing IO in such large chunks
anyway.
This patch changes the wasm-wasi-component language module to read the
request buffer in 32MiB chunks (this matches the original 'wasm'
language module).
We are still limited to a 4GiB address space and can only upload files a
little under 4GiB.
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Run from the repository root like
$ rustfmt --edition 2021 src/wasm-wasi-component/src/lib.rs
Also manually fix up some overly long comments.
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This is used by the rustfmt program to format Rust code according to the
rules contained in this file.
Currently we just set the line width limit to 80 characters to match our
C code.
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This is required to actually _build_ the 'wasm-wasi-componet' language
module.
The nxt_wasm_wc_app_conf_t structure consists of the component name, e.g
my_component.wasm, this is required. It also consists of an object to
store the directories that are allowed access to by the component, this
is optional.
The bulk of the configuration infrastructure will be added in a
subsequent commit.
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This is the first commit in adding WebAssembly Component Model language
module support.
This just adds a new NXT_APP_WASM_WC type, required by subsequent
commits.
The WC stands for WASI_COMPONENT
This new module will have a type of 'wasm-wasi-component'.
Link: <https://github.com/nginx/unit/issues/1098>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Add more entries for Andrei and Konstantin and an entry for Dan.
Reviewed-by: Dan Callahan <d.callahan@f5.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This is a simple temporary fix (doesn't address the underlying problem)
for an issue reported by a user on GitHub whereby downloading of files
from a PHP application would cause the router process to crash.
This is actually a generic problem that will affect anything sending
data via nxt_unit_response_write().
This is just a simple fix for the 1.32 release, after which the full
correct fix will be worked out.
Link: <https://github.com/nginx/unit/issues/1125>
Reported-by: rustedsword <https://github.com/rustedsword>
Co-developed-by: rustedsword <https://github.com/rustedsword>
Tested-by: rustedsword <https://github.com/rustedsword>
Tested-by: Andrew Clayton <a.clayton@nginx.com>
Reviewed-by: Zhidao Hong <z.hong@f5.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
A user on GitHub reported an issue when trying to build/install the
nodejs language module.
Doing a
$ ./configure nodejs --node=/usr/bin/node --npm=/usr/bin/npm --node-gyp=/usr/bin/node-gyp
$ make install
was throwing the following error
mv build/src//usr/bin/node/unit-http-g/unit-http-1.31.1.tgz build//usr/bin/node-unit-http-g.tar.gz
mv: cannot move 'build/src//usr/bin/node/unit-http-g/unit-http-1.31.1.tgz' to 'build//usr/bin/node-unit-http-g.tar.gz': No such file or directory
make: *** [build/Makefile:2061: build//usr/bin/node-unit-http-g.tar.gz] Error 1
The fact that we're using the path given by --node= to then use as
directory locations seems erroneous.
But rather than risk breaking existing expectations the simple fix is to
just use build/src in the destination path above to match that of the
source.
These paths were added in some previous commits, and the missing 'src/'
component looks like an oversight.
After this commit both the following work
$ ./configure nodejs --node-gyp=/usr/lib/node_modules/bin/node-gyp-bin/node-gyp --local=/opt/unit/node
$ ./configure nodejs --node=/usr/bin/node --node-gyp=/usr/lib/node_modules/npm/bin/node-gyp-bin/node-gyp --local=/opt/unit/node
Reported-by: ruspaul013 <https://github.com/ruspaul013>
Tested-by: ruspaul013 <https://github.com/ruspaul013>
Fixes: 0ee8de554 ("Fixed Makefile target for NodeJS.")
Fixes: c84948386 ("Node.js: fixing module global installation.")
Reviewed-by: Timo Stark <t.stark@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This wraps chown(2) but takes the user/owner and group as strings.
It's a little long winded as it uses the thread safe versions of
getpwnam()/getgrname() which require a little more work.
This function will be used by the following commit that allows to set
the permissions of the Unix domain control socket.
We need to cast uid & gid to long in the call to nxt_thread_log_alert()
to appease clang-ast as it's adamant that uid/gid are unsigned ints, but
chown(2) takes -1 for these values to indicate don't change this item,
and it'd be nice to show them in the error message.
Note that getpwnam()/getgrname() don't define "not found" as an error as
per their man page
The formulation given above under "RETURN VALUE" is from POSIX.1-2001.
It does not call "not found" an error, and hence does not specify what
value errno might have in this situation. But that makes it impossible
to recognize errors. One might argue that according to POSIX errno
should be left unchanged if an entry is not found. Experiments on var‐
ious UNIX-like systems show that lots of different values occur in this
situation: 0, ENOENT, EBADF, ESRCH, EWOULDBLOCK, EPERM, and probably
others.
Thus if we log an error from these functions we can end up with the
slightly humorous error message
2024/02/12 15:15:12 [alert] 99404#99404 getpwnam_r("noddy", ...) failed (0: Success) (User not found) while creating listening socket on unix:/opt/unit/control.unit.sock
Reviewed-by: Zhidao Hong <z.hong@f5.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
With the previous commit which introduced the use of the
NXT_CONF_VLDT_REQUIRED flag, we no longer need to do this separate
validation, it's only purpose was to check if the three uidmap/gidmap
settings had been provided.
Reviewed-by: Zhidao Hong <z.hong@f5.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Use the NXT_CONF_VLDT_REQUIRED flag on the app_procmap members. These
three settings are required.
These are for the uidmap & gidmap settings in the config.
Suggested-by: Zhidao HONG <z.hong@f5.com>
Reviewed-by: Zhidao Hong <z.hong@f5.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Andrei reported an issue on arm64 where he was seeing the following
error message when running the tests
2024/01/17 18:32:31.109 [error] 54904#54904 "gidmap" field has an entry with "size": 1, but for unprivileged unit it must be 1.
This error message is guarded by the following if statement
if (nxt_slow_path(m.size > 1)
Turns out size was indeed > 1, in this case it was 289356276058554369,
m.size is defined as a nxt_int_t, which on arm64 is actually 8 bytes,
but was being printed as a signed int (4 bytes) and by chance/undefined
behaviour comes out as 1.
But why is size so big? In this case it should have just been 1 with a
config of
'gidmap': [{'container': 0, 'host': os.getegid(), 'size': 1}],
This is due to nxt_int_t being 64bits on arm64 but using a conf type of
NXT_CONF_MAP_INT which means in nxt_conf_map_object() we would do (using
our m.size variable as an example)
ptr = nxt_pointer_to(data, map[i].offset);
...
ptr->i = num;
Where ptr is a union pointer and is now pointing at our m.size
Next we set m.size to the value of num (which is 1 in this case), via
ptr->i where i is a member of that union of type int.
So here we are setting a 64bit memory location (nxt_int_t on arm64)
through a 32bit (int) union alias, this means we are only setting the
lower half (4) of the bytes.
Whatever happens to be in the upper 4 bytes will remain, giving us our
exceptionally large value.
This is demonstrated by this program
#include <stdio.h>
#include <stdint.h>
int main(void)
{
int64_t num = -1; /* All 1's in two's complement */
union {
int32_t i32;
int64_t i64;
} *ptr;
ptr = (void *)#
ptr->i32 = 1;
printf("num : %lu / %ld\n", num, num);
ptr->i64 = 1;
printf("num : %ld\n", num);
return 0;
}
$ make union-32-64-issue
cc union-32-64-issue.c -o union-32-64-issue
$ ./union-32-64-issue
num : 18446744069414584321 / -4294967295
num : 1
However that is not the only issue, because the members of
nxt_clone_map_entry_t were specified as nxt_int_t's on the likes of
x86_64 this would be a 32bit signed integer. However uid/gids on Linux
at least are defined as unsigned integers, so a nxt_int_t would not be
big enough to hold all potential values.
We could make the nxt_uint_t's but then we're back to the above union
aliasing problem.
We could just set the memory for these variables to 0 and that would
work, however that's really just papering over the problem.
The right thing is to use a large enough sized type to store these
things, hence the previously introduced nxt_cred_t. This is an int64_t
which is plenty large enough.
So we switch the nxt_clone_map_entry_t structure members over to
nxt_cred_t's and use NXT_CONF_MAP_INT64 as the conf type, which then
uses the right sized union member in nxt_conf_map_object() to set these
variables.
Reported-by: Andrei Zeliankou <zelenkov@nginx.com>
Reviewed-by: Zhidao Hong <z.hong@f5.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This is a generic type to represent a uid_t/gid_t on Linux when user
namespaces are in use.
Technically this only needs to be an unsigned int, but we make it an
int64_t so we can make use of the existing NXT_CONF_MAP_INT64 type.
This will be used in subsequent commits.
Reviewed-by: Zhidao Hong <z.hong@f5.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
On GH, @tonychuuy reported an issue when using Units 'share' action they
would get the following error in the unit log
2024/01/15 17:53:41 [error] 49#52 *103 file "/var/www/html/public/vendor/telescope/app.css" has changed while sending response to a client
This would happen when trying to serve files over a certain size and the
requested file would not be sent.
This is due to a somewhat bogus check in
nxt_http_static_buf_completion()
I say bogus because it's not clear what the check is trying to
accomplish and the error message is not entirely accurate either.
The check in question goes like
n = pread(file->fd, buf, size, offset);
return n;
...
if (n != size) {
if (n >= 0) {
/* log file changed error and finish */
/* >> Problem is here << */
}
/* log general error and finish */
}
If the number of bytes read is not what we asked for and is > -1 (i.e
not an error) then it says the file has changed, but really it only
checks if the file has _shrunk_ (we can't get back _more_ bytes than we
asked for) since it was stat'd.
This is what happens
recvfrom(22, "GET /tfile HTTP/1.1\r\nHost: local"..., 2048, 0, NULL, NULL) = 82
openat(AT_FDCWD, "/mnt/9p/tfile", O_RDONLY|O_NONBLOCK) = 23
newfstatat(23, "", {st_mode=S_IFREG|0644, st_size=149922, ...}, AT_EMPTY_PATH) = 0
We get a request from a client, open the requested file and stat(2) it to
get the file size.
We would then go into a pread/writev loop reading the file data and
sending it to the client until it's all been sent.
However what was happening in this case was this (showing a dummy file
of 149922 bytes)
pread64(23, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 131072, 0) = 61440
write(2, "2024/01/17 15:30:50 [error] 1849"..., 109) = 109
We wanted to read 131072 bytes but only read 61440 bytes, the above
check triggered and the file transfer was aborted and the above error
message logged.
Normally for a regular file you will only get less bytes than asked for
if the read call is interrupted by a signal or you're near the end of
file.
There is however at least another situation where this may happen, if
the file in question is being served from a network filesystem.
It turns out that was indeed the case here, the files where being served
over the 9P filesystem protocol. Unit was running in a docker container
in an Ubuntu VM under Windows/WSL2 and the files where being passed
through to the VM from Windows over 9P.
Whatever the intention of this check, it is clearly causing issues in
real world scenarios.
If it was really desired to check if the had changed since it was
opened/stat'd then it would require a different methodology and be a
patch for another day. But as it stands this current check does more
harm than good, so lets just remove it.
With it removed we now get for the above test file
recvfrom(22, "GET /tfile HTTP/1.1\r\nHost: local"..., 2048, 0, NULL, NULL) = 82
openat(AT_FDCWD, "/mnt/9p/tfile", O_RDONLY|O_NONBLOCK) = 23
newfstatat(23, "", {st_mode=S_IFREG|0644, st_size=149922, ...}, AT_EMPTY_PATH) = 0
mmap(NULL, 135168, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f367817b000
pread64(23, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 131072, 0) = 61440
pread64(23, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 18850, 61440) = 18850
writev(22, [{iov_base="HTTP/1.1 200 OK\r\nLast-Modified: "..., iov_len=171}, {iov_base="\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., iov_len=61440}, {iov_base="\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., iov_len=18850}], 3) = 80461
pread64(23, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 69632, 80290) = 61440
pread64(23, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 8192, 141730) = 8192
close(23) = 0
writev(22, [{iov_base="\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., iov_len=61440}, {iov_base="\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., iov_len=8192}], 2) = 69632
So we can see we do two pread(2)s's and a writev(2), then another two
pread(2)s and another writev(2) and all the file data has been read and
sent to the client.
Reported-by: tonychuuy <https://github.com/tonychuuy>
Link: <https://en.wikipedia.org/wiki/9P_(protocol)>
Fixes: 08a8d1510 ("Basic support for serving static files.")
Closes: https://github.com/nginx/unit/issues/1064
Reviewed-by: Zhidao Hong <z.hong@f5.com>
Reviewed-by: Andrei Zeliankou <zelenkov@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Fix up a mixture of different names/email addresses people have used.
You can always see the original names/addresses used by passing
--no-mailmap to the various git commands.
See gitmailmap(5)
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Map her GitHub noreply address to her @f5 one.
You can always see the original address used by passing --no-mailmap to
the various git commands.
Note: We don't always need the name field, but we're keeping this file
consistent and alphabetically ordered on first name...
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Coverity picked up a potential issue with the previous commit d9f5f1fb7
("Ruby: Handle response field arrays") in that a size_t could wrap
around to SIZE_MAX - 1.
This would happen if we were given an empty array of header values.
Fixes: d9f5f1fb7 ("Ruby: Handle response field arrays")
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
@xeron on GitHub reported an issue whereby with a Rails 7.1 application
they were getting the following error
2023/10/22 20:57:28 [error] 56#56 [unit] #8: Ruby: Wrong header entry 'value' from application
2023/10/22 20:57:28 [error] 56#56 [unit] #8: Ruby: Failed to run ruby script
After some back and forth debugging it turns out rack was trying to send
back a header comprised of an array of values. E.g
app = Proc.new do |env|
["200", {
"Content-Type" => "text/plain",
"X-Array-Header" => ["Item-1", "Item-2"],
}, ["Hello World\n"]]
end
run app
It seems this became a possibility in rack v3.0[0]
So along with a header value type of T_STRING we need to also allow
T_ARRAY.
If we get a T_ARRAY we need to build up the header field using the given
values.
E.g
"X-Array-Header" => ["Item-1", "", "Item-3", "Item-4"],
becomes
X-Array-Header: Item-1; ; Item-3; Item-4
[0]: <https://github.com/rack/rack/blob/main/UPGRADE-GUIDE.md?plain=1#L26>
Reported-by: Ivan Larionov <xeron.oskom@gmail.com>
Closes: <https://github.com/nginx/unit/issues/974>
Link: <https://github.com/nginx/unit/pull/998>
Tested-by: Timo Stark <t.stark@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Due to GH making a mess of merge commits, it used Danielle's personal
email address for the merge, it also used a generic GH address for the
committer but we can't do anything about that. However we can fix the
'Author' email address.
If for some reason you want to see the original names/addresses used you
can generally pass --no-mailmap to git commands.
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
In nxt_php_execute() it is possible we could bail out before cleaning up
the FILE * representing the PHP script to execute.
At this point we only need to call fclose(3) on it.
We could have possibly moved the opening of this file to later in the
function, but it is probably good to bail out as early as possible if we
can't open it.
This was found by Coverity.
Fixes: bebc03c72 ("PHP: Implement better error handling.")
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
On GitHub, @RomainMou reported an issue whereby HTTP header field values
where being incorrectly reported as non-ascii by the Python .isacii()
method.
For example, using the following test application
def application(environ, start_response):
t = environ['HTTP_ASCIITEST']
t = "'" + t + "'" + " (" + str(len(t)) + ")"
if t.isascii():
t = t + " [ascii]"
else:
t = t + " [non-ascii]"
resp = t + "\n\n"
start_response("200 OK", [("Content-Type", "text/plain")])
return (bytes(resp, 'latin1'))
You would see the following
$ curl -H "ASCIITEST: $" http://localhost:8080/
'$' (1) [non-ascii]
'$' has an ASCII code of 0x24 (36).
The initial idea was to adjust the second parameter to the
PyUnicode_New() call from 255 to 127. This unfortunately had the
opposite effect.
$ curl -H "ASCIITEST: $" http://localhost:8080/
'$' (1) [ascii]
Good. However...
$ curl -H "ASCIITEST: £" http://localhost:8080/
'£' (2) [ascii]
Not good. Let's take a closer look at this.
'£' is not in basic ASCII, but is in extended ASCII with a value of 0xA3
(163). Its UTF-8 encoding is 0xC2 0xA3, hence the length of 2 bytes
above.
$ strace -s 256 -e sendto,recvfrom curl -H "ASCIITEST: £" http://localhost:8080/
sendto(5, "GET / HTTP/1.1\r\nHost: localhost:8080\r\nUser-Agent: curl/8.0.1\r\nAccept: */*\r\nASCIITEST: \302\243\r\n\r\n", 92, MSG_NOSIGNAL, NULL, 0) = 92
recvfrom(5, "HTTP/1.1 200 OK\r\nContent-Type: text/plain\r\nServer: Unit/1.30.0\r\nDate: Mon, 22 May 2023 12:44:11 GMT\r\nTransfer-Encoding: chunked\r\n\r\n12\r\n'\302\243' (2) [ascii]\n\n\r\n0\r\n\r\n", 102400, 0, NULL, NULL) = 160
'£' (2) [ascii]
So we can see curl sent it UTF-8 encoded '\302\243\' which is C octal
escaped UTF-8 for 0xC2 0xA3, and we got the same back. But it should not
be marked as ASCII.
When doing PyUnicode_New(size, 127) it sets the buffer as ASCII. So we
need to use another function and that function would appear to be
PyUnicode_DecodeCharmap()
Which creates an Unicode object with the correct ascii/non-ascii
properties based on the character encoding.
With this function we now get
$ curl -H "ASCIITEST: $" http://localhost:8080/
'$' (1) [ascii]
$ curl -H "ASCIITEST: £" http://localhost:8080/
'£' (2) [non-ascii]
and for good measure
$ curl -H "ASCIITEST: $ £" http://localhost:8080/
'$ £' (4) [non-ascii]
$ curl -H "ASCIITEST: $" -H "ASCIITEST: £" http://localhost:8080/
'$, £' (5) [non-ascii]
PyUnicode_DecodeCharmap() does require having the full string upfront so
we need to build up the potentially comma separated header field values
string before invoking this function.
I did not want to touch the Python 2.7 code (which may or may not even
be affected by this) so kept these changes completely isolated from
that, hence a slight duplication with the for () loop.
Python 2.7 was sunset on January 1st 2020[0], so this code will
hopefully just disappear soon anyway.
I also purposefully didn't touch other code that may well have similar
issues (such as the HTTP header field names) if we ever get issue
reports about them, we'll deal with them then.
[0]: <https://www.python.org/doc/sunset-python-2/>
Link: <https://docs.python.org/3/c-api/unicode.html>
Closes: <https://github.com/nginx/unit/issues/868>
Reported-by: RomainMou <https://github.com/RomainMou>
Tested-by: RomainMou <https://github.com/RomainMou>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This is a preparatory patch for fixing an issue with the encoding of
http header field values.
This patch simply moves the nxt_unit_sptr_get() to the top of the
function where we will need it in the next commit.
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This was inadvertently removed in 76086d6d ("Wasm: Allow to set the HTTP
response status.")
Fixes: 76086d6d ("Wasm: Allow to set the HTTP response status.")
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
On Github, @rlandgrebe reported an issue when trying to rewrite URLs
that contained query strings.
With the PHP language module we were in fact segfaulting (SIGSEGV) in
libphp
[93960.462952] unitd[20940]: segfault at 7f307cef6476 ip 00007f2f81a94577 sp 00007fff28a777d0 error 4 in libphp-8.2.so[7f2f818df000+2fd000] likely on CPU 0 (core 0, socket 0)
#0 0x00007f2abd494577 in php_default_treat_data (arg=1, str=0x0,
destArray=<optimized out>)
at /usr/src/debug/php-8.2.10-1.fc38.x86_64/main/php_variables.c:488
488 if (c_var && *c_var) {
(gdb) p c_var
$1 = 0x7f2bb8880676 <error: Cannot access memory at address 0x7f2bb8880676>
This was when trying to get the query string which somehow is pointing
off into the woods.
This gdb debug session when doing rewrite basically shows the core of
the issue
(gdb) x /64bs req->fields
...
0x7f7eaaaa8090: "GET"
0x7f7eaaaa8094: "HTTP/1.1"
0x7f7eaaaa809d: "::1"
0x7f7eaaaa80a1: "::1"
0x7f7eaaaa80a5: "8080"
0x7f7eaaaa80aa: "localhost"
0x7f7eaaaa80b4: "/test?q=a"
0x7f7eaaaa80be: "/test"
...
(gdb) p target_pos
$4 = (void *) 0x7f7eaaaa80b4
(gdb) p query_pos
$6 = (void *) 0x7f7eaaaa6af6
(gdb) p r->args->start
$8 = (u_char *) 0x7f7ea4002b02 "q=a HTTP/1.1\r\nHost: localhost:8080\r\nUser-Agent: curl/8.0.1\r\nAccept: */*\r\n\r\n"
(gdb) p r->target.start
$9 = (u_char *) 0x7f7ea40040c0 "/test?q=a"
That last address, 0x7f7ea40040c0, looks out of wack, it should be
smaller than r->args->start.
That results in a calculation in nxt_router_prepare_msg()
if (r->args->start != NULL) {
query_pos = nxt_pointer_to(target_pos,
r->args->start - r->target.start);
nxt_unit_sptr_set(&req->query, query_pos);
} else {
that goes negative that then is stored in req->query.offset which is a
uint32_t and so wraps backwards from UINT_MAX to give us an offset of a
little under 4GiB, hence the above invalid memory access.
All this happens due to in nxt_http_rewrite() if we have a URL with a
query string, we create a new memory allocation to store the transformed
URL and query string.
We set r->target to point to this new allocation, but we also need to
point r->args->start to the start of the query string in this new
allocation.
Reported-by: René Landgrebe <https://github.com/rlandgrebe>
Tested-by: René Landgrebe <https://github.com/rlandgrebe>
Tested-by: Liam Crilly <liam.crilly@nginx.com>
Fixes: 14d6d97b ("HTTP: added basic URI rewrite.")
Closes: <https://github.com/nginx/unit/issues/964>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
When trying to upload files to the luw-upload-reflector demo[0] above a
certain size that would mean Unit would need to make more than two calls
to the request_handler function in the Wasm module we would get the
following error from wasmtime and the upload would stall on the third
call to the request_handler
WASMTIME ERROR: failed to call function [->wasm_request_handler]
error while executing at wasm backtrace:
0: 0x5ce2 - <unknown>!memcpy
1: 0x7df - luw_req_buf_append
at /home/andrew/src/unit-wasm/src/c/libunit-wasm.c:308:14
2: 0x3a1 - luw_request_handler
at /home/andrew/src/unit-wasm/examples/c/luw-upload-reflector.c:110:3
Caused by:
wasm trap: out of bounds memory access
This was due to ->content_off (the offset of where the actual body
content starts in the request structure/memory) being some overly large
value.
This was largely down to me being an idiot!
Before calling the loop that makes the calls to the request_handler we
would calculate the new offset, which is now just the size of the
request structure as we don't re-send all the HTTP meta data and headers
etc. However because this value is in the request structure which is in
the shared memory and we use this same memory for requests and
responses, when we make a response we overwrite this request structure
with the response structure, so our ->content_off is now some wacked out
value when we make the next call to the request_handler.
To fix this we just need to reset ->content_off each time round the
loop.
There's also no point in setting ->nfields to 0, it has the same issue
as above, but doesn't get re-used by the Wasm module anyway.
[0]: <https://github.com/nginx/unit-wasm/blob/main/examples/c/luw-upload-reflector.c>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This commit enables WebAssembly modules to set the HTTP response status
to something other than the previously hard coded '200 OK'.
To do this they can make a call to nxt_wasm_set_resp_status() providing
the required status code.
If this function isn't called the status code defaults to '200 OK'. The
WebAssembly module can also return -1 from the request_handler function
as a short cut to signal a '500 Internal Server Error'.
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
As reported by @andypost on GitHub, if you try to build Unit on a system
that uses musl libc (such as Alpine Linux) with clang then you get the
following
clang -c -pipe -fPIC -fvisibility=hidden -O -W -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -fstrict-aliasing -Wstrict-overflow=5 -Wmissing-prototypes -Werror -g -I src -I build/include \
\
\
-o build/src/nxt_socketpair.o \
-MMD -MF build/src/nxt_socketpair.dep -MT build/src/nxt_socketpair.o \
src/nxt_socketpair.c
In file included from src/nxt_socketpair.c:8:
src/nxt_socket_msg.h:138:17: error: comparison of integers of different signs: 'unsigned long' and 'long' [-Werror,-Wsign-compare]
cmsg = CMSG_NXTHDR(&msg, cmsg))
^~~~~~~~~~~~~~~~~~~~~~~
/usr/include/sys/socket.h:358:44: note: expanded from macro 'CMSG_NXTHDR'
__CMSG_LEN(cmsg) + sizeof(struct cmsghdr) >= __MHDR_END(mhdr) - (unsigned char *)(cmsg) \
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from src/nxt_socketpair.c:8:
src/nxt_socket_msg.h:177:17: error: comparison of integers of different signs: 'unsigned long' and 'long' [-Werror,-Wsign-compare]
cmsg = CMSG_NXTHDR(&msg, cmsg))
^~~~~~~~~~~~~~~~~~~~~~~
/usr/include/sys/socket.h:358:44: note: expanded from macro 'CMSG_NXTHDR'
__CMSG_LEN(cmsg) + sizeof(struct cmsghdr) >= __MHDR_END(mhdr) - (unsigned char *)(cmsg) \
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 errors generated.
make: *** [build/Makefile:261: build/src/nxt_socketpair.o] Error 1
GCC works fine, it seems to have some smarts so that it doesn't give
warnings on system header files.
This seems to be a long standing issue with musl libc (bad casting in
the CMSG_NXTHDR macro) and the workaround employed by several projects
is to disable the -Wsign-compare clang warning for the code in question.
So, that's what we do. We wrap the CMSG_NXTHDR macro in a function, so
we can use the pre-processor in it to selectively disable the warning.
Link: <https://github.com/dotnet/runtime/issues/16438>
Link: <https://git.openembedded.org/meta-openembedded/tree/meta-oe/recipes-devtools/breakpad/breakpad/0001-Turn-off-sign-compare-for-musl-libc.patch>
Link: <https://github.com/dotnet/corefx/blob/57ff88bb75a0/src/Native/Unix/System.Native/pal_networking.c#L811-L829>
Link: <https://patchwork.yoctoproject.org/project/oe/patch/20220407191438.3696227-1-stefan@datenfreihafen.org/>
Closes: <https://github.com/nginx/unit/issues/936>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Due to the sandboxed nature of WebAssembly, by default WASM modules
don't have any access to the underlying filesystem.
There is however a capabilities based mechanism[0] for allowing such
access.
This adds a config option to the 'wasm' application type;
'access.filesystem' which takes an array of directory paths that are
then made available to the WASM module. This access works recursively,
i.e everything under a specific path is allowed access to.
Example config might look like
"access" {
"filesystem": [
"/tmp",
"/var/tmp"
]
}
The actual mechanism used allows directories to be mapped differently in
the guest. But at the moment we don't support that and just map say /tmp
to /tmp. This can be revisited if it's something users clamour for.
Network sockets are another resource that may be controlled in this
manner, for example there is a wasi_config_preopen_socket() function,
however this requires the runtime to open the network socket then
effectively pass this through to the guest.
This is something that can be revisited in the future if users desire
it.
[0]:
<https://github.com/bytecodealliance/wasmtime/blob/main/docs/WASI-capabilities.md>
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This exposes various WebAssembly language module specific options.
The application type is "wasm".
There is a "module" option that is required, this specifies the full
path to the WebAssembly module to be run. This module should be in
binary format, i.e a .wasm file.
There are also currently eight function handlers that can be specified.
Three of them are _required_
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 five _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.
Example config
"applications": {
"luw-echo-request": {
"type": "wasm",
"module": "/path/to/unit-wasm/examples/c/luw-echo-request.wasm",
"request_handler": "luw_request_handler",
"malloc_handler": "luw_malloc_handler",
"free_handler": "luw_free_handler",
"module_init_handler": "luw_module_init_handler",
"module_end_handler": "luw_module_end_handler",
}
}
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
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>
