Can be reproduced by test/test_variables.py::test_variables_dynamic_arguments
with enabled UndefinedBehaviorSanitizer:
src/nxt_http_request.c:961:17: runtime error: applying zero offset to null pointer
#0 0x1050d95a4 in nxt_http_arguments_parse nxt_http_request.c:961
#1 0x105102bf8 in nxt_http_var_arg nxt_http_variables.c:621
#2 0x104f95d74 in nxt_var_interpreter nxt_var.c:507
#3 0x104f98c98 in nxt_tstr_query nxt_tstr.c:265
#4 0x1050abfd8 in nxt_router_access_log_writer nxt_router_access_log.c:194
#5 0x1050d81f4 in nxt_http_request_close_handler nxt_http_request.c:838
#6 0x104fcdc48 in nxt_event_engine_start nxt_event_engine.c:542
#7 0x104fba838 in nxt_thread_trampoline nxt_thread.c:126
#8 0x18133e030 in _pthread_start+0x84 (libsystem_pthread.dylib:arm64e+0x7030)
#9 0x181338e38 in thread_start+0x4 (libsystem_pthread.dylib:arm64e+0x1e38)
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/nxt_http_request.c:961:17
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
Found by UndefinedBehaviorSanitizer:
src/nxt_random.c:151:31: runtime error: left shift of 140 by 24 places cannot be represented in type 'int'
#0 0x104f78968 in nxt_random nxt_random.c:151
#1 0x104f58a98 in nxt_shm_open nxt_port_memory.c:377
#2 0x10503e24c in nxt_controller_conf_send nxt_controller.c:617
#3 0x105041154 in nxt_controller_process_request nxt_controller.c:1109
#4 0x104fcdc48 in nxt_event_engine_start nxt_event_engine.c:542
#5 0x104f27254 in main nxt_main.c:35
#6 0x180fbd0dc (<unknown module>)
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/nxt_random.c:151:31
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
Can be reproduced by test/test_rewrite.py::test_rewrite_njs
with enabled UndefinedBehaviorSanitizer:
src/nxt_http_js.c:169:52: runtime error: applying zero offset to null pointer
#0 0x10255b044 in nxt_http_js_ext_get_args nxt_http_js.c:169
#1 0x102598ad0 in njs_value_property njs_value.c:1175
#2 0x10259c2c8 in njs_vm_object_prop njs_vm.c:1398
#3 0x102559d74 in nxt_js_call nxt_js.c:445
#4 0x1023c0da0 in nxt_tstr_query nxt_tstr.c:276
#5 0x102516ec4 in nxt_http_rewrite nxt_http_rewrite.c:56
#6 0x1024fd86c in nxt_http_request_action nxt_http_request.c:565
#7 0x1024d71b0 in nxt_h1p_request_body_read nxt_h1proto.c:998
#8 0x1023f5c48 in nxt_event_engine_start nxt_event_engine.c:542
#9 0x1023e2838 in nxt_thread_trampoline nxt_thread.c:126
#10 0x18133e030 in _pthread_start+0x84 (libsystem_pthread.dylib:arm64e+0x7030)
#11 0x181338e38 in thread_start+0x4 (libsystem_pthread.dylib:arm64e+0x1e38)
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/nxt_http_js.c:169:52
Same fix was introduced in NJS:
<http://hg.nginx.org/njs/rev/4fba78789fe4>
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
Otherwise, undefined behaviour will be triggered.
Can be reproduced by test/test_routing.py::test_routes_match_host_empty
with enabled UndefinedBehaviorSanitizer:
src/nxt_http_route.c:2141:17: runtime error: applying zero offset to null pointer
#0 0x100562588 in nxt_http_route_test_rule nxt_http_route.c:2091
#1 0x100564ed8 in nxt_http_route_handler nxt_http_route.c:1574
#2 0x10055188c in nxt_http_request_action nxt_http_request.c:570
#3 0x10052b1a0 in nxt_h1p_request_body_read nxt_h1proto.c:998
#4 0x100449c38 in nxt_event_engine_start nxt_event_engine.c:542
#5 0x100436828 in nxt_thread_trampoline nxt_thread.c:126
#6 0x18133e030 in _pthread_start+0x84 (libsystem_pthread.dylib:arm64e+0x7030)
#7 0x181338e38 in thread_start+0x4 (libsystem_pthread.dylib:arm64e+0x1e38)
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/nxt_http_route.c:2141:17
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
We already use dependabot for security related patches, by default.
This change adds a dependabot.yml configuration file that explicitly
enables the service to manage versions of Actions in GitHub Actions.
This ensures that Actions like `setup-go` are updated timely.
This change does not affect how Dependabot manages versions for Go,
Rust, etc. The file can be used to configure that for additional
package managers and languages in the future, if desired.
This variable is _appended_ to the main CFLAGS variable and allows
setting extra compiler options at make time. E.g
$ make EXTRA_CFLAGS="..." ...
Useful for quickly testing various extra warning flags.
Suggested-by: Alejandro Colomar <alx@kernel.org>
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This adds a help target to the Makefile in the repository root that
shows what variables are available to control the make/build behaviour.
It currently looks like
$ make help
Variables to control make/build behaviour:
make V=1 ... - Enables verbose output
make D=1 ... - Enables debug builds (-O0)
make E=0 ... - Disables -Werror
Variables can be combined.
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Having -Werror enabled all the time when developing can be a nuisance,
allow to disable it by passing E=0 to make, e.g
$ make E=0 ...
This will set -Wno-error overriding the previously set -Werror.
Co-developed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
One issue you have when trying to debug Unit under say GDB is that at
the default optimisation level we use of -O (-O1) the compiler will
often optimise things out which means they are not available for
inspection in the debugger.
This patch allows you to pass 'D=1' to make, e.g
$ make D=1 ...
Which will set -O0 overriding the previously set -O, basically disabling
optimisations, we could use -Og, but the clang(1) man page says this is
best and it seems to not cause any issues when debugging GCC generated
code.
Co-developed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This makes use of the infrastructure introduced in a previous commit, to
pretty print the make output when building the wasm language module.
You can still get the old verbose output with
$ make V=1 ...
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This makes use of the infrastructure introduced in a previous commit, to
pretty print the make output when building the Ruby language module.
You can still get the old verbose output with
$ make V=1 ...
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This makes use of the infrastructure introduced in a previous commit, to
pretty print the make output when building the Python language module.
You can still get the old verbose output with
$ make V=1 ...
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This makes use of the infrastructure introduced in a previous commit, to
pretty print the make output when building the PHP language module.
You can still get the old verbose output with
$ make V=1 ...
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This makes use of the infrastructure introduced in a previous commit, to
pretty print the make output when building the Perl language module.
You can still get the old verbose output with
$ make V=1 ...
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This makes use of the infrastructure introduced in a previous commit, to
pretty print the make output when building the Java language module.
You can still get the old verbose output with
$ make V=1 ...
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This makes use of the infrastructure introduced in the previous commit
to pretty print the make output when building the Unit core and the C
test programs.
When building Unit the output now looks like
VER build/include/nxt_version.h (NXT_VERSION)
VER build/include/nxt_version.h (NXT_VERNUM)
CC build/src/nxt_lib.o
CC build/src/nxt_gmtime.o
...
CC build/src/nxt_cgroup.o
AR build/lib/libnxt.a
CC build/src/nxt_main.o
LD build/sbin/unitd
SED build/share/man/man8/unitd.8
I'm sure you'll agree that looks much nicer!
You can still get the old verbose output with
$ make V=1 ...
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
The idea is rather than printing out the full compiler/linker etc
command for each recipe e.g
cc -c -pipe -fPIC -fvisibility=hidden -O0 -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wno-strict-aliasing -Wmissing-prototypes -g -I src -I build/include \
\
\
-o build/src/nxt_cgroup.o \
-MMD -MF build/src/nxt_cgroup.dep -MT build/src/nxt_cgroup.o \
src/nxt_cgroup.c
Print a clearer abbreviated message e.g the above becomes
CC build/src/nxt_cgroup.o
This vastly reduces the noise when compiling and most of the time you
don't need to see the full command being executed.
This also means that warnings etc show up much more clearly.
You can still get the old verbose output by passing V=1 to make e.g
$ make V=1 ...
NOTE: With recent versions of make(1) you can get this same, verbose,
behaviour by using the --debug=print option.
This introduces the following message types
CC Compiling a source file to an object file.
AR Producing a static library, .a archive file.
LD Producing a dynamic library, .so DSO, or executable.
VER Writing version information.
SED Running sed(1).
All in all this improves the developer experience.
Subsequent commits will make use of this in the core and modules.
NOTE: This requires GNU make for which we check. On OpenIndiana/illumos
we have to use gmake(1) (GNU make) anyway as the illumos make doesn't
work with our Makefile as it is. Also macOS seems to generally install
GNU make.
We could make it work (probably) on other variants of make, but the
complexity starts increasing exponentially.
In fact we still print the abbreviated messages in the verbose output so
you can still do
$ make | grep ^" [A-Z]"
on other makes to effectively get the same output.
Co-developed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This causes signed integer & pointer overflow to have a defined
behaviour of wrapping according to two's compliment. I.e INT_MAX will
wrap to INT_MIN and vice versa.
This is mainly to cover existing cases, not an invitation to add more.
Cc: Dan Callahan <d.callahan@f5.com>
Suggested-by: Alejandro Colomar <alx@kernel.org>
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Aliasing is essentially when you access the same memory via different
types.
If the compiler knows this doesn't happen it can make some
optimisations.
There is however code in Unit, for example in the wasm language module
and the websocket code that may fall foul of strict-aliasing rules.
(For the wasm module I explicitly disable it there)
In auto/cc/test for GCC we have
NXT_CFLAGS="$NXT_CFLAGS -O"
...
# -O2 enables -fstrict-aliasing and -fstrict-overflow.
#NXT_CFLAGS="$NXT_CFLAGS -O2"
#NXT_CFLAGS="$NXT_CFLAGS -Wno-strict-aliasing"
So with GCC by default we effectively compile with -fno-strict-aliasing.
For clang we have this
NXT_CFLAGS="$NXT_CFLAGS -O"
...
#NXT_CFLAGS="$NXT_CFLAGS -O2"
...
NXT_CFLAGS="$NXT_CFLAGS -fstrict-aliasing"
(In _clang_, -fstrict-aliasing is always enabled by default)
So in clang we always build with -fstrict-aliasing. I don't think this
is the best idea, building with something as fundamental as this
disabled in one compiler and enabled in another.
This patch adjusts the Clang side of things to match that of GCC. I.e
compile with -fno-strict-aliasing. It also explicitly sets
-fno-strict-aliasing for GCC, which is what we were getting anyway but
lets be explicit about it.
Cc: Dan Callahan <d.callahan@f5.com>
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Expand on the comment on why we don't enable -Wstrict-overflow=5 on GCC.
Link: <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96658>
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This is what -Wextra used to be called, but any version of GCC or Clang
in at least the last decade has -Wextra.
Cc: Dan Callahan <d.callahan@f5.com>
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
We really only support building Unit with GCC and Clang.
Cc: Dan Callahan <d.callahan@f5.com>
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
We really only support building Unit with GCC and Clang.
Cc: Dan Callahan <d.callahan@f5.com>
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
We only really support building Unit with GCC and Clang.
Cc: Dan Callahan <d.callahan@f5.com>
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
We don't run on Windows and only really support compiling Unit with GCC
and Clang.
Cc: Dan Callahan <d.callahan@f5.com>
Co-developed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
The variables accessed with JS template literal should not be cacheable.
Since it is parsed by njs engine, Unit can't create indexes on these
variables for caching purpose. For example:
{
"format": "`{bodyLength:\"${vars.body_bytes_sent}\",status:\"${vars.status}\"}\n`"
}
The variables like the above are not cacheable.
Closes: https://github.com/nginx/unit/issues/1169
I changed a setting and now GitHub will recognize both the legacy
numberless version, and the newer version with UserID.
The with-UserID version will be used by the any changes stemming from
the GitHub GUI.
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
Signed-off-by: Dylan Arbour <d.arbour@f5.com>
This adds a GitHub CI workflow for the new wasm-wasi-component language
module.
Some things of note.
1) We need to special case 'wasm-wasi-component' in the 'Output build
metadata' section as we are splitting the module names on '-' to
split them into name and version.
2) Apart from needing to tell bindgen about the njs include paths, we
also need to explicitly specify which version of clang to use to
work around an issue with multiple versions of clang installed.
Link: <https://gitlab.freedesktop.org/mesa/mesa/-/issues/7268>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
To enable tests that require privileged root access, this commit tests
with `sudo`. The Java and Python jobs have additional permissions
issues, so they are also configured and made with `sudo`.
A small permissions fix is required before running tests to allow
non-root users to execute within the `/home/runner` directory.
This change also removes the custom directories that were required
without root access.
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
Signed-off-by: Dylan Arbour <d.arbour@f5.com>
In nxt_router_temp_conf() we have
rtcf = nxt_mp_zget(mp, sizeof(nxt_router_conf_t));
if (nxt_slow_path(rtcf == NULL)) {
goto fail;
}
If rtcf is NULL then we do
fail:
if (rtcf->tstr_state != NULL) {
nxt_tstr_state_release(rtcf->tstr_state);
}
In which case we will dereference the NULL pointer rtcf.
This patch re-works the goto labels to make them more specific to their
intended purpose and ensures we are freeing things which have been
allocated.
This was found by the clang static analyser.
Reviewed-by: Zhidao Hong <z.hong@f5.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
p is not used again before returning from the function.
Found by the clang static analyser.
Reviewed-by: Zhidao Hong <z.hong@f5.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This is unused, yet a community member just spent time finding and
fixing a bug in it only to be told it's unused.
Just get rid of the thing.
Link: <https://github.com/nginx/unit/pull/963>
Reviewed-by: Zhidao Hong <z.hong@f5.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
If we compile Unit with -Wstrict-overflow=5 (as we do with clang) then
we get the following warning
cc -c -pipe -fPIC -fvisibility=hidden -O0 -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wstrict-overflow=5 -Wmissing-prototypes -g -I src -I build/include \
\
\
-o build/src/nxt_conf.o \
-MMD -MF build/src/nxt_conf.dep -MT build/src/nxt_conf.o \
src/nxt_conf.c
src/nxt_conf.c: In function ‘nxt_conf_json_parse_value’:
src/nxt_conf.c:1444:5: warning: assuming signed overflow does not occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-Wstrict-overflow]
1444 | if (nxt_fast_path((ch - '0') <= 9)) {
|
Does this actually cause an issue?... well, yes. Using this minimal test
config to show the problem
{
"listeners": {
"[::1]:8080": {
"pass": --100
}
}
}
With the above if () statement that triggers the warning, my assumption
here is that we only want a digit now. '0' - '9'.
ch is a u_char, however if ch is any character with an ASCII code < 48
('0') e.g if ch is '-' (45) then we get 45 - 48 = -3, through arithmetic
conversion, which makes the if () statement true (when it shouldn't) then
at some point we get the following error returned from the controller
{
"error": "Memory allocation failed."
}
Instead of the expected
{
"error": "Invalid JSON.",
"detail": "A valid JSON value is expected here. It must be either a literal (null, true, or false), a number, a string (in double quotes \"\"), an array (with brackets []), or an object (with braces {}).",
"location": {
"offset": 234,
"line": 15,
"column": 27
}
}
Casting the result of (ch - '0') to u_char resolves this issue, this
makes the above calculation come out as 253 (relying on unsigned integer
wraparound) which was probably the intended way for it to work.
Reviewed-by: Zhidao Hong <z.hong@f5.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
-o is not available on macOS 12.7 at least, and it's what homebrew seems
to support still.
Also, the proposed switch seems to be used already in the codebase.
This extends the approach used for debian-based packages in 3f805bc64e
to rpm as well. Notable change for both deb and rpm packaging is to use
CFLAGS as defined in the build/Makefile, and not pass them from the
environment which might not be there (as is the case for rpm).
While at it, stop passing CFLAGS in the install phase, as it should no
longer invoke builds (see d54af163c4).
The rpm part was overlooked in 7a6405566c, since testing was not done
on the platforms where problem manifested itself, notably Amazon Linux
2023 and Fedora 38+.
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>
We need to redefine CFLAGS to drop missing-prototypes as warning since
third-party code such as wasmtime fails to build from source when
building a debian package. This happens only for packages because we
use DPKG_EXPORT_BUILDFLAGS=1 propagating build flags to the environment,
so cargo build picks it up as well. Since we have no control over
third-party code, the easiest solution is to disable this warning.
Python applications are used only to generate responses here and can be
replaced by applications written in any other language. While the
"_python" prefix is used to indicate that the file contains tests
specific to the Python module.
These tests cause router crash when run with AddressSanitizer:
=================================================================
==77196==ERROR: AddressSanitizer: heap-use-after-free on address 0x60c000079340 at pc 0x55d56b132d4b bp 0x7f8cc7f346b0 sp 0x7f8cc7f346a0
READ of size 1 at 0x60c000079340 thread T1
#0 0x55d56b132d4a in nxt_openssl_conn_io_shutdown src/nxt_openssl.c:1466
#1 0x55d56b0f6a25 in nxt_h1p_closing src/nxt_h1proto.c:2069
#2 0x55d56b1009a6 in nxt_h1p_shutdown src/nxt_h1proto.c:2038
#3 0x55d56b1014c3 in nxt_h1p_request_close src/nxt_h1proto.c:1718
#4 0x55d56b1045c0 in nxt_http_request_close_handler src/nxt_http_request.c:864
#5 0x55d56b104988 in nxt_http_request_done src/nxt_http_request.c:795
#6 0x55d56b0ba0c3 in nxt_event_engine_start src/nxt_event_engine.c:542
#7 0x55d56b0dcac2 in nxt_router_thread_start src/nxt_router.c:3645
#8 0x55d56b0b421b in nxt_thread_trampoline src/nxt_thread.c:126
#9 0x7f8ccab95ac2 (/lib/x86_64-linux-gnu/libc.so.6+0x94ac2)
#10 0x7f8ccac2784f (/lib/x86_64-linux-gnu/libc.so.6+0x12684f)
