| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
On some Python 3.11 systems, 3.11.9 & 3.11.10, we were seeing a crash
triggered by Py_Finalize() in nxt_python_atexit() when running one of
our pytests, namely
test/test_python_factory.py::test_python_factory_invalid_callable_value
2024/09/12 15:07:29 [alert] 5452#5452 factory "wsgi_invalid_callable" in module "wsgi" can not be called to fetch callable
Fatal Python error: none_dealloc: deallocating None: bug likely caused by a refcount error in a C extension
Python runtime state: finalizing (tstate=0x00007f560b88a718)
Current thread 0x00007f560bde7ad0 (most recent call first):
<no Python frame>
2024/09/12 15:07:29 [alert] 5451#5451 app process 5452 exited on signal 6 (core dumped)
This was due to
obj = PyDict_GetItemString(PyModule_GetDict(module), callable);
in nxt_python_set_target() which returns a *borrowed* reference, then
due to the test meaning this is a `None` object we `goto fail` and call
Py_DECREF(obj);
which then causes `Py_Finalize()` to blow up.
The simple fix is to just increment its reference count before the `goto
fail`.
Note: This problem only showed up under (the various versions of Python
we test on); 3.11.9 & 3.11.10. It doesn't show up under; 3.6, 3.7, 3.9,
3.10, 3.12
Cc: Konstantin Pavlov <thresh@nginx.com>
Closes: https://github.com/nginx/unit/issues/1413
Fixes: a9aa9e76d ("python: Support application factories")
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
When the client closes the connection before the upstream,
the proxy's error handler was calling cleanup operation like
peer close and request close twice, this fix ensures the cleanup
is performed only once, improving proxy stability.
Closes: https://github.com/nginx/unit/issues/828
|
|
While the C based wasm language module inherits the process environment
the Rust based wasm-wasi-component language module did not.
One upshot of this is that with wasm-wasi-component you don't get access
to any environment variables specified in the Unit configuration.
wasm-wasi-component was based on wasmtime 17.0.0. This capability wasn't
added to the wasmtime-crate until version 20.0.0.
Now that wasm-wasi-component has been updated to a newer wasmtime-crate
we can enable this functionality.
Closes: https://github.com/nginx/unit/issues/1312
[ Commit message - Andrew ]
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
Signed-off-by: Ava Hahn <a.hahn@f5.com>
|
|
This was found via ASan.
Given a listener address like ":" (or any address where the first
character is a colon) we can end up under-reading the addr->start
buffer here
if (nxt_slow_path(*(buf + length - 1) == '.')) {
due to length (essentially the position of the ":" in the string) being
0.
Seeing as any address that starts with a ":" is invalid Unit config
wise, we should simply reject the address if length == 0 in
nxt_sockaddr_inet_parse().
Link: <https://clang.llvm.org/docs/AddressSanitizer.html>
Signed-off-by: Arjun <pkillarjun@protonmail.com>
[ Commit message - Andrew ]
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
This feature allows users to specify conditions to check if one
route is matched. It is used the same way as the "if" option in
the access log.
Example:
{
"match": {
"if": "`${headers['User-Agent'].split('/')[0] == 'curl'}`"
},
"action": {
"return": 204
}
}
|
|
|
|
This nxt_tstr_cond_t will be reused for the feature of adding "if"
option to the "match" object. The two "if" options have the same usage.
|
|
|
|
|
|
|
|
|
|
|
|
Initially, variable query was designed to accomodate both synchronous
and asynchronous operations. However, upon consideration of actual
requirements, we recognized that asynchronous support was not needed.
The refactoring ensures that the success or failure of the variable
query operation is now directly indicated by its return value. This
change streamlines the function's usage and enhances code clarity,
as it facilitates immediate error handling without the need for
asynchronous callbacks or additional error checking functions.
Note the patch only works for Unit native variables but not njs
variables.
|
|
@oopsoop2 on GitHub reported a performance issue related to the default
listen(2) backlog size of 511 on nginx. They found that increasing it
helped, nginx has a config option to configure this.
They would like to be able to do the same on Unit (which also defaults
to 511 on some systems). This seems reasonable.
NOTE: On Linux before commit 97c15fa38 ("socket: Use a default listen
backlog of -1 on Linux") we defaulted to 511. Since that commit we
default to the Kernels default, which before 5.4 is 128 and after is
4096.
This adds a new per-listener 'backlog' config option, e.g
{
"listeners": {
"[::1]:8080": {
"pass": "routes",
"backlog": 1024
},
}
...
}
This doesn't effect the control socket.
Closes: https://github.com/nginx/unit/issues/1384
Reported-by: <https://github.com/oopsoop2>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
On FreeBSD, OpenBSD & macOS we use a default listen(2) backlog of -1
which means use the OS's default value.
On Linux (and others) we used a hard coded value of 511, presumably due
to this comment
/* Linux, Solaris, and NetBSD treat negative value as 0. */
On Linux (at least since 2.4), this is wrong, Linux treats -1 (and so
on) as use the OS's default (net.core.somaxconn). See this code in
net/socket.c::__sys_listen()
if ((unsigned int)backlog > somaxconn)
backlog = somaxconn;
On Linux prior to 5.4 somaxconn defaulted to 128, since 5.4 it defaults
to 4096.
We've had complaints that a listen backlog of 511 is too small. This
would help in those cases.
Unless they are on an old Kernel, in which case it's worse, but then the
plan is to also make this configurable. This would effect RHEL 8, which
is based on 4.10, however they seem to set somaxconn to 2048, so that's
fine.
Another advantage of using -1 is that we will automatically keep up to
date with the kernels default value.
Before this change
$ ss -tunxlp | grep unit
Netid State Recv-Q Send-Q Local Address:Port Peer Address:Port Process
u_str LISTEN 0 511 /opt/unit/control.unit.sock.tmp 4302333 * 0 users:(("unitd",pid=18290,fd=6),("unitd",pid=18289,fd=6),("unitd",pid=18287,fd=6))
tcp LISTEN 0 511 127.0.0.1:8080 0.0.0.0:* users:(("unitd",pid=18290,fd=12))
tcp LISTEN 0 511 [::1]:8080 [::]:* users:(("unitd",pid=18290,fd=11))
After
$ ss -tunxlp | grep unit
Netid State Recv-Q Send-Q Local Address:Port Peer Address:Port Process
u_str LISTEN 0 4096 /opt/unit/control.unit.sock.tmp 5408464 * 0 users:(("unitd",pid=132442,fd=6),("unitd",pid=132441,fd=6),("unitd",pid=132439,fd=6))
tcp LISTEN 0 4096 127.0.0.1:8080 0.0.0.0:* users:(("unitd",pid=132442,fd=12))
tcp LISTEN 0 4096 [::1]:8080 [::]:* users:(("unitd",pid=132442,fd=11))
Link: <https://github.com/nginx/unit/issues/1384>
Link: <https://lore.kernel.org/netdev/20191030163620.140387-1-edumazet@google.com/>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
Unit generally creates an extra number of router threads (to handle
client connections, not incl the main thread) to match the number of
available CPUs.
There are cases when this can go wrong, e.g on a high CPU count machine
and Unit is being effectively limited to a few CPUs via the cgroups cpu
controller. So Unit may create a large number of router threads when
they are only going to effectively run on a couple of CPUs or so.
There may be other cases where you would like to tweak the number of
router threads, depending on your workload.
As it turns out it looks like it was intended to be made configurable
but was just never hooked up to the config system.
This adds a new '/settings/listen_threads' config option which can be
set like
{
"listen": {
...
},
"settings": {
"listen_threads": 2,
...
},
...
}
Before this patch (on a four cpu system)
$ ps -efL | grep router
andrew 419832 419829 419832 0 5 Aug12 pts/10 00:00:00 unit: router
andrew 419832 419829 419833 0 5 Aug12 pts/10 00:00:00 unit: router
andrew 419832 419829 419834 0 5 Aug12 pts/10 00:00:00 unit: router
andrew 419832 419829 445145 0 5 03:31 pts/10 00:00:00 unit: router
andrew 419832 419829 445146 0 5 03:31 pts/10 00:00:00 unit: router
After, with a threads setting of 2
$ ps -efL | grep router
andrew 419832 419829 419832 0 3 Aug12 pts/10 00:00:00 unit: router
andrew 419832 419829 419833 0 3 Aug12 pts/10 00:00:00 unit: router
andrew 419832 419829 419834 0 3 Aug12 pts/10 00:00:00 unit: router
Closes: https://github.com/nginx/unit/issues/1042
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
At startup, the unit router process creates a number of threads, it
tries to create the same number of threads (not incl the main thread) as
there are 'cpus' in the system.
On Linux the number of available cpus is determined via a call to
sysconf(_SC_NPROCESSORS_ONLN);
in a lot of cases this produces the right result, i.e. on a four cpu
system this will return 4.
However this can break down if unit has been restricted in the cpus it's
allowed to run on via something like cpuset()'s and/or
sched_setaffinity(2).
For example, on a four 'cpu' system, starting unit will create an extra
4 router threads
$ /opt/unit/sbin/unitd
$ ps -efL | grep router
andrew 234102 234099 234102 0 5 17:00 pts/10 00:00:00 unit: router
andrew 234102 234099 234103 0 5 17:00 pts/10 00:00:00 unit: router
andrew 234102 234099 234104 0 5 17:00 pts/10 00:00:00 unit: router
andrew 234102 234099 234105 0 5 17:00 pts/10 00:00:00 unit: router
andrew 234102 234099 234106 0 5 17:00 pts/10 00:00:00 unit: router
Say we want to limit unit to two cpus, i.e.
$ taskset -a -c 2-3 /opt/unit/sbin/unitd
$ ps -efL | grep router
andrew 235772 235769 235772 0 5 17:08 pts/10 00:00:00 unit: router
andrew 235772 235769 235773 0 5 17:08 pts/10 00:00:00 unit: router
andrew 235772 235769 235774 0 5 17:08 pts/10 00:00:00 unit: router
andrew 235772 235769 235775 0 5 17:08 pts/10 00:00:00 unit: router
andrew 235772 235769 235776 0 5 17:08 pts/10 00:00:00 unit: router
So despite limiting unit to two cpus
$ grep Cpus_allowed_list /proc/235772/status
Cpus_allowed_list: 2-3
It still created 4 threads, probably not such an issue in this case, but
if we had a 64 'cpu' system and wanted to limit unit two cpus, then we'd
have 64 threads vying to run on two cpus and with our spinlock
implementation this can cause a lot of thread scheduling and congestion
overhead.
Besides, our intention is currently to create nr router threads == nr
cpus.
To resolve this, on Linux at least, this patch makes use of
sched_getaffinity(2) to determine what cpus unit is actually allowed to
run on.
We still use the result of
sysconf(_SC_NPROCESSORS_ONLN);
as a fallback, we also use its result to allocate the required cpuset
size (where sched_getaffinity() will store its result) as the standard
cpu_set_t only has space to store 1023 cpus.
So with this patch if we try to limit unit to two cpus we now get
$ taskset -a -c 2-3 /opt/unit/sbin/unitd
$ ps -efL | grep router
andrew 236887 236884 236887 0 3 17:20 pts/10 00:00:00 unit: router
andrew 236887 236884 236888 0 3 17:20 pts/10 00:00:00 unit: router
andrew 236887 236884 236889 0 3 17:20 pts/10 00:00:00 unit: router
This also applies to the likes of docker, if you run docker with the
--cpuset-cpus="" option, unit will now create a number of router threads
that matches the cpu count specified.
Perhaps useful if you are running a number of unit docker instances on a
high cpu count machine.
Link: <https://github.com/nginx/unit/issues/1042>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
Fixes: 707f4ef8 ("status: Show list of loaded language modules")
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
As the comment for 'Memory-only buffers' says
"... it is equal to offsetof(nxt_buf_t, file.pos)"
and
"... that is it is nxt_buf_t without file and mmap part"
Those are at odds with each other, 'file.pos' comes _after_ 'file' in
the nxt_buf_t structure.
Fix the 'offset()' bit of the comment to reflect that and to match the
relevant macro
#define NXT_BUF_MEM_SIZE offsetof(nxt_buf_t, file)
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
When querying the '/status' node in the control API, display the list of
currently loaded modules.
So we now get something like
{
"modules": {
"python": [
{
"version": "3.12.3",
"lib": "/opt/unit/modules/python.unit.so"
},
{
"version": "3.12.1",
"lib": "/opt/unit/modules/python-3.12.1.unit.so"
}
],
"wasm": {
"version": "0.1",
"lib": "/opt/unit/modules/wasm.unit.so"
},
"wasm-wasi-component": {
"version": "0.1",
"lib": "/opt/unit/modules/wasm_wasi_component.unit.so"
}
},
...
}
This can be useful for debugging to show exactly what modules Unit has
loaded _and_ from where.
Closes: https://github.com/nginx/unit/issues/1343
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
The nxt_app_lang_module_t structure contains various bits of information
as obtained from the nxt_app_module_t structure that language modules
define.
One bit of information that is in the nxt_app_module_t but not in the
nxt_app_lang_module_t is the language module name.
Having this name flowed through will be useful for displaying the loaded
language modules in the /status endpoint.
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
In nxt_status_get() call nxt_conf_set_member() multiple times to set the
main /status json sections.
Previously this used hard coded values, 0, 1, 2 etc, if you wanted to
change the order or insert new sections it could mean renumbering all
these.
Instead use a variable to track this index which starts at 0 and is
simply incremented in each call of nxt_conf_set_member().
Currently this is only for the main outer sections, but can be
replicated for inner sections if required.
This is a preparatory patch for adding a new "modules" section at the
top.
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
This is yet more missed constification, due in this case to me searching
for 'static nxt_str_t ' but these only having a single space after the
type...
Anyway no problem, this can be a preparatory patch for adding further
/status information...
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
These somehow got missed in my previous constification patches...
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
Previously, the certificate deletion only handled string type
certificates, causing issues when certificates were specified
as an array in the configuration.
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
|
|
Adds support for the app factory pattern to the Python language module.
A factory is a callable that returns a WSGI or ASGI application object.
Unit does not support passing arguments to factories.
Setting the `factory` option to `true` instructs Unit to treat the
configured `callable` as a factory.
For example:
"my-app": {
"type": "python",
"path": "/srv/www/",
"module": "hello",
"callable": "create_app",
"factory": true
}
This is similar to other WSGI / ASGI servers. E.g.,
$ uvicorn --factory hello:create_app
$ gunicorn 'hello:create_app()'
The factory setting defaults to false.
Closes: https://github.com/nginx/unit/issues/1106
Link: <https://github.com/nginx/unit/pull/1336#issuecomment-2179381605>
[ Commit message - Dan / Minor code tweaks - Andrew ]
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
njs changed strings API so now instead of njs_vm_value_string_set() used
njs_vm_value_string_create() as a drop-in replacement.
Link: <https://github.com/nginx/njs/commit/5730d5ffe23a4965c001d873695d22005fcfa588>
|
|
This issue was found with oss-fuzz.
==18420==WARNING: MemorySanitizer: use-of-uninitialized-value
#0 0x55dd798a5797 in nxt_vsprintf unit/src/nxt_sprintf.c:163:31
#1 0x55dd798d5bdb in nxt_conf_vldt_error unit/src/nxt_conf_validation.c:1525:11
#2 0x55dd798dd4cd in nxt_conf_vldt_var unit/src/nxt_conf_validation.c:1560:16
#3 0x55dd798dd4cd in nxt_conf_vldt_if unit/src/nxt_conf_validation.c:1592:16
#4 0x55dd798d55f4 in nxt_conf_vldt_object unit/src/nxt_conf_validation.c:2815:23
#5 0x55dd798d6f84 in nxt_conf_vldt_access_log unit/src/nxt_conf_validation.c:3426:11
#6 0x55dd798d55f4 in nxt_conf_vldt_object unit/src/nxt_conf_validation.c:2815:23
#7 0x55dd798d47bd in nxt_conf_validate unit/src/nxt_conf_validation.c:1421:11
#8 0x55dd79871c82 in LLVMFuzzerTestOneInput unit/fuzzing/nxt_json_fuzz.c:67:5
#9 0x55dd79770620 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:614:13
#10 0x55dd7975adb4 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:327:6
#11 0x55dd7976084a in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:862:9
#12 0x55dd7978cc42 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
#13 0x7e8192213082 in __libc_start_main /build/glibc-SzIz7B/glibc-2.31/csu/libc-start.c:308:16
#14 0x55dd7975188d in _start
Uninitialized value was created by an allocation of 'error.i' in the stack frame
#0 0x55dd798dd42b in nxt_conf_vldt_var unit/src/nxt_conf_validation.c:1557:5
#1 0x55dd798dd42b in nxt_conf_vldt_if unit/src/nxt_conf_validation.c:1592:16
The issue was in nxt_tstr_test() where we create an error message with
nxt_sprintf(), where this error message is then later used with the
'%s' format specifier which expects a nul-terminated string, but by
default nxt_sprintf() doesn't nul-terminate, you must use the '%Z'
specifier to signify a '\0' at the end of the string.
Signed-off-by: Arjun <pkillarjun@protonmail.com>
Co-developed-by: Zhidao HONG <z.hong@f5.com>
Signed-off-by: Zhidao HONG <z.hong@f5.com>
Link: <https://github.com/google/oss-fuzz>
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
[ Commit message/subject - Andrew ]
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
These somehow got missed in my previous constification patches...
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
These somehow got missed in my previous constification patches...
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
This is a temporary support for chunked request bodies by converting
to Content-Length. This allows for processing of such requests until
a more permanent solution is developed.
A new configuration option "chunked_transform" has been added to enable
this feature. The option can be set as follows:
{
"settings": {
"chunked_transform": true
}
}
By default, this option is set to false, which retains the current
behaviour of rejecting chunked requests with a '411 Length Required'
status code.
Please note that this is an experimental implementation.
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
|
|
It's prepared for the subsequent patch.
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
|
|
Previously, Unit didn't move the buffer pointer when parsing chunked
data because the buffer was not used after sent. It's used for upstream
response. With the new requirement to support request chunked body,
the buffer might be used for pipeline request, so it's necessary to move
the pos pointer while parsing.
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
|
|
They are more readable.
And we had a mix of both styles; there wasn't really a consistent style.
Tested-by: Andrew Clayton <a.clayton@nginx.com>
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
|
|
Build systems should not attempt to create $runstatedir (or anything
under it). Doing so causes warnings in packaging systems, such as in
Alpine Linux, as reported by Andy.
But unitd(8) can be configured to be installed under /opt, or other
trees, where no directories exist before hand. Expecting that the user
creates the entire directory trees that unit will need is a bit
unreasonable. Instead, let's just create any directories that we need,
with all their parents, at run time.
Fixes: 57fc9201cb91 ("Socket: Created control socket & pid file directories.")
Link: <https://github.com/nginx/unit/issues/742>
Reported-by: Andy Postnikov <apostnikov@gmail.com>
Tested-by: Andy Postnikov <apostnikov@gmail.com>
Tested-by: Andrew Clayton <a.clayton@nginx.com>
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
Acked-by: Konstantin Pavlov <thresh@nginx.com>
Cc: Liam Crilly <liam@nginx.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
|
|
The previous code attempted to mkdir(""), that is an empty string.
Since "/" necessarily exists, just goto out_free.
Fixes: 57fc9201cb91 ("Socket: Created control socket & pid file directories.")
Link: <https://github.com/nginx/unit/issues/742>
Tested-by: Andy Postnikov <apostnikov@gmail.com>
Tested-by: Andrew Clayton <a.clayton@nginx.com>
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
Cc: Liam Crilly <liam@nginx.com>
Cc: Konstantin Pavlov <thresh@nginx.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
|
|
This refactor isn't very appealing alone, but it prepares the code for
the following commits.
Link: <https://github.com/nginx/unit/issues/742>
Tested-by: Andy Postnikov <apostnikov@gmail.com>
Tested-by: Andrew Clayton <a.clayton@nginx.com>
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
Cc: Liam Crilly <liam@nginx.com>
Cc: Konstantin Pavlov <thresh@nginx.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
|
|
That is, accept "/", or relative path names of a single byte.
Fixes: e2b53e16c60b ("Added "rootfs" feature.")
Tested-by: Andy Postnikov <apostnikov@gmail.com>
Tested-by: Andrew Clayton <a.clayton@nginx.com>
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
|
|
Tested-by: Andy Postnikov <apostnikov@gmail.com>
Tested-by: Andrew Clayton <a.clayton@nginx.com>
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
|
|
This avoids breaking a long line.
Tested-by: Andy Postnikov <apostnikov@gmail.com>
Tested-by: Andrew Clayton <a.clayton@nginx.com>
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
|
|
That branch was to avoid an infinite loop on the slash. However, we can
achieve the same by using a +1 to make sure we advance at least 1 byte
in each iteration.
Tested-by: Andy Postnikov <apostnikov@gmail.com>
Tested-by: Andrew Clayton <a.clayton@nginx.com>
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
|
|
"all" is too generic of an attribute to be meaningful. In the context
of mkdir(), "parents" is used for this meaning, as in mkdir -p, so it
should be more straightforward to readers.
Tested-by: Andy Postnikov <apostnikov@gmail.com>
Tested-by: Andrew Clayton <a.clayton@nginx.com>
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
|
|
"dirname" is the usual way to refer to the directory part of a path
name. See for example dirname(1), or the dirname builtin in several
languages. Also, in the context of mkdir(), "parents" is used to refer
to mkdir -p, which is too similar to "parent", so it can lead to
confusion.
Tested-by: Andy Postnikov <apostnikov@gmail.com>
Tested-by: Andrew Clayton <a.clayton@nginx.com>
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
|
|
This was found via MSan.
In nxt_http_fields_hash() we setup a nxt_lvlhsh_query_t structure and
initialise a couple of its members.
At some point we call
lhq->proto->alloc(lhq->pool, nxt_lvlhsh_bucket_size(lhq->proto));
Which in this case is
void *
nxt_lvlhsh_alloc(void *data, size_t size)
{
return nxt_memalign(size, size);
}
So even though lhq.ppol wasn't previously initialised we don't actually
use it in that particular function.
However MSan triggers on the fact that we are passing an uninitialised
value into that function.
Indeed, compilers will generally complain about such things, e.g
/* u.c */
struct t {
void *p;
int len;
};
static void test(void *p __attribute__((unused)), int len)
{
(void)len;
}
int main(void)
{
struct t t;
t.len = 42;
test(t.p, t.len);
return 0;
}
GCC and Clang will produce a -Wuninitialized warning.
But they won't catch the following...
/* u2.c */
struct t {
void *p;
int len;
};
static void _test(void *p __attribute__((unused)), int len)
{
(void)len;
}
static void test(struct t *t)
{
_test(t->p, t->len);
}
int main(void)
{
struct t t;
t.len = 42;
test(&t);
return 0;
}
Which is why we don't get a compiler warning about lhq.pool.
In this case initialising lhg.pool even though we don't use it here
seems like the right thing to do and maybe compilers will start being
able to catch these in the future.
Actually GCC with -fanalyzer does catch the above
$ gcc -Wall -Wextra -O0 -fanalyzer u2.c
u2.c: In function ‘test’:
u2.c:15:9: warning: use of uninitialized value ‘*t.p’ [CWE-457] [-Wanalyzer-use-of-uninitialized-value]
15 | _test(t->p, t->len);
| ^~~~~~~~~~~~~~~~~~~
‘main’: events 1-3
|
| 18 | int main(void)
| | ^~~~
| | |
| | (1) entry to ‘main’
| 19 | {
| 20 | struct t t;
| | ~
| | |
| | (2) region created on stack here
|......
| 23 | test(&t);
| | ~~~~~~~~
| | |
| | (3) calling ‘test’ from ‘main’
|
+--> ‘test’: events 4-5
|
| 13 | static void test(struct t *t)
| | ^~~~
| | |
| | (4) entry to ‘test’
| 14 | {
| 15 | _test(t->p, t->len);
| | ~~~~~~~~~~~~~~~~~~~
| | |
| | (5) use of uninitialized value ‘*t.p’ here
|
Signed-off-by: Arjun <pkillarjun@protonmail.com>
Link: <https://clang.llvm.org/docs/MemorySanitizer.html>
[ Commit message - Andrew ]
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
This allows you to then define strings like
static const nxt_str_t my_str = nxt_string("string");
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
Previously, the REQUEST_URI within Unit could be modified,
for example, during uri rewriting. We decide to make $request_uri
immutable and pass constant REQUEST_URI to applications.
Based on the new requirement, we remove `r->target` rewriting
in the rewrite module.
Closes: https://github.com/nginx/unit/issues/916
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
Signed-off-by: Zhidao HONG <z.hong@f5.com>
|
|
This change is required for the next commit, after which target
and r->target may be different. Before the next patch, target and
r->target would be the same.
No functional changes.
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
Signed-off-by: Zhidao HONG <z.hong@f5.com>
|
|
Declaring a 0-sized array (e.g 'char arr[0];') as the last member of a
structure is a GNU extension that was used to implement flexible array
members (FAMs) before they were standardised in C99 as simply '[]'.
The GNU extension itself was introduced to work around a hack of
declaring 1-sized arrays to mean a variable-length object. The advantage
of the 0-sized (and true FAMs) is that they don't count towards the size
of the structure.
Unit already declares some true FAMs, but it also declared some 0-sized
arrays.
Converting these 0-sized arrays to true FAMs is not only good for
consistency but will also allow better compiler checks now (as in a C99
FAM *must* be the last member of a structure and the compiler will warn
otherwise) and in the future as doing this fixes a bunch of warnings
(treated as errors in Unit by default) when compiled with
-O2 -Warray-bounds -Wstrict-flex-arrays -fstrict-flex-arrays=3
(Note -Warray-bounds is enabled by -Wall and -Wstrict-flex-arrays seems
to also be enabled via -Wall -Wextra, the -02 is required to make
-fstrict-flex-arrays more effective, =3 is the default on at least GCC
14)
such as
CC build/src/nxt_upstream.o
src/nxt_upstream.c: In function ‘nxt_upstreams_create’:
src/nxt_upstream.c:56:18: error: array subscript i is outside array bounds of ‘nxt_upstream_t[0]’ {aka ‘struct nxt_upstream_s[]’} [-Werror=array-bounds=]
56 | string = nxt_str_dup(mp, &upstreams->upstream[i].name, &name);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from src/nxt_upstream.c:9:
src/nxt_upstream.h:55:48: note: while referencing ‘upstream’
55 | nxt_upstream_t upstream[0];
| ^~~~~~~~
Making our flexible array members proper C99 FAMs and ensuring any >0
sized trailing arrays in structures are really normal arrays will allow
to enable various compiler options (such as the above and more) that
will help keep our array usage safe.
Changing 0-sized arrays to FAMs should have no effect on structure
layouts/sizes (they both have a size of 0, although doing a sizeof() on
a FAM will result in a compiler error).
Looking at pahole(1) output for the nxt_http_route_ruleset_t structure
for the [0] and [] cases...
$ pahole -C nxt_http_route_ruleset_t /tmp/build/src/nxt_http_route.o
typedef struct {
uint32_t items; /* 0 4 */
/* XXX 4 bytes hole, try to pack */
nxt_http_route_rule_t * rule[]; /* 8 0 */
/* size: 8, cachelines: 1, members: 2 */
/* sum members: 4, holes: 1, sum holes: 4 */
/* last cacheline: 8 bytes */
} nxt_http_route_ruleset_t;
$ pahole -C nxt_http_route_ruleset_t build/src/nxt_http_route.o
typedef struct {
uint32_t items; /* 0 4 */
/* XXX 4 bytes hole, try to pack */
nxt_http_route_rule_t * rule[]; /* 8 0 */
/* size: 8, cachelines: 1, members: 2 */
/* sum members: 4, holes: 1, sum holes: 4 */
/* last cacheline: 8 bytes */
} nxt_http_route_ruleset_t;
Also checking with the size(1) command on the effected object files
shows no changes to their sizes
$ for file in build/src/nxt_upstream.o \
build/src/nxt_upstream_round_robin.o \
build/src/nxt_h1proto.o \
build/src/nxt_http_route.o \
build/src/nxt_http_proxy.o \
build/src/python/*.o; do \
size -G /tmp/${file} $file; echo; done
text data bss total filename
640 418 0 1058 /tmp/build/src/nxt_upstream.o
640 418 0 1058 build/src/nxt_upstream.o
text data bss total filename
929 351 0 1280 /tmp/build/src/nxt_upstream_round_robin.o
929 351 0 1280 build/src/nxt_upstream_round_robin.o
text data bss total filename
11707 8281 16 20004 /tmp/build/src/nxt_h1proto.o
11707 8281 16 20004 build/src/nxt_h1proto.o
text data bss total filename
8319 3101 0 11420 /tmp/build/src/nxt_http_route.o
8319 3101 0 11420 build/src/nxt_http_route.o
text data bss total filename
1495 1056 0 2551 /tmp/build/src/nxt_http_proxy.o
1495 1056 0 2551 build/src/nxt_http_proxy.o
text data bss total filename
4321 2895 0 7216 /tmp/build/src/python/nxt_python_asgi_http-python.o
4321 2895 0 7216 build/src/python/nxt_python_asgi_http-python.o
text data bss total filename
4231 2266 0 6497 /tmp/build/src/python/nxt_python_asgi_lifespan-python.o
4231 2266 0 6497 build/src/python/nxt_python_asgi_lifespan-python.o
text data bss total filename
12051 6090 8 18149 /tmp/build/src/python/nxt_python_asgi-python.o
12051 6090 8 18149 build/src/python/nxt_python_asgi-python.o
text data bss total filename
28 1963 432 2423 /tmp/build/src/python/nxt_python_asgi_str-python.o
28 1963 432 2423 build/src/python/nxt_python_asgi_str-python.o
text data bss total filename
5818 3518 0 9336 /tmp/build/src/python/nxt_python_asgi_websocket-python.o
5818 3518 0 9336 build/src/python/nxt_python_asgi_websocket-python.o
text data bss total filename
4391 2089 168 6648 /tmp/build/src/python/nxt_python-python.o
4391 2089 168 6648 build/src/python/nxt_python-python.o
text data bss total filename
9095 5909 152 15156 /tmp/build/src/python/nxt_python_wsgi-python.o
9095 5909 152 15156 build/src/python/nxt_python_wsgi-python.o
Link: <https://lwn.net/Articles/908817/>
Link: <https://people.kernel.org/kees/bounded-flexible-arrays-in-c>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
