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>
Commit 4fc50258b ("ci: Be more specific when to run the main Unit
checks") limited when the checks for the main ci run, on pushes to
master.
It should have done the same for pull-requests.
Fixes: 4fc50258b ("ci: Be more specific when to run the main Unit checks")
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This patch does a number of things to help when failing to apply a new
configuration.
* The error body from the Unit control API is displayed which can have
useful troubleshooting information (when the version of curl supports it).
* When using the EDIT option, the temporary file with unapplied changes is
preserved so that the user can edit it again without losing their work.
* Editing JavaScript modules no longer requires that module to have been
enabled.
* Failure to apply edited JavaScript modules now rolls-back to the previous
configuration instead of deleting the module.
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
ci-dev-distro-compiler.yaml already limits itself to running only when
relevant things are updated.
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
The lists of files being unpacked are mostly useless but take a
significant amount of lines and bytes in e.g. CI jobs. E.g. in rhel9
packaging job, it's 39680 lines just for the unpacking of
wasmtime-v11.0.1-src, as compared to total 48945 lines of output.
dpkg-query -W will show information about the package if any other
package references it, even when the queried package is not installed.
The fix is to query for an actual status of a needed build dependency.
This adds a workflow for building Unit under Fedora Rawhide and Alpine
Edge with both GCC and Clang.
These are the development branches from which releases are cut.
This usually consists of the latest versions of software and will
hopefully catch new compiler issues and API breakages in the various
languages we support.
With Alpine and Clang that also gives us musl libc + clang coverage.
On Alpine we don't build the wasm and wasm-wasi-component modules,
mainly as this would require messing around with all the rust stuff and
building wasmtime from source (as there's no musl libc based packages)
and the wasm module is pretty small, any new compiler issues would
hopefully show up in the rest.
We _do_ build the wasm module with gcc and clang on Fedora. But not
wasm-wasi-component in the interests of time. Can be added at a later
date if deemed necessary.
We don't build the Perl language module on Fedora with clang due to the
Fedora (and probably Red Hat) Perl CFLAGS having incompatible with clang
flags.
We probably could work around it if we really wanted to, but not sure
it's worth it and on Red Hat/Fedora, GCC _is_ the system compiler.
On Alpine we also don't build the nodejs and go language modules as
there's nothing that actually gets compiled there and the _main_ reason
for building on Alpine is to get musl libc + clang coverage.
We're also not bothering with njs for now... can be revisited at a
later date.
Also no pytests, these should be well covered via other workflows for
example by running on latest Alpine releases.
Closes: https://github.com/nginx/unit/issues/949
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
You can always see the original names/addresses used by passing
--no-mailmap to the various git commands.
See gitmailmap(5)
Reviewed-by: Ava Hahn <a.hahn@f5.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
* fix a few misspellings in unitctl CI workflow
* add unit testing job
* exclude unitd integration test from unit tests
* add workflow dispatch trigger
* add calls to get workflow dispatch version
Signed-off-by: Ava Hahn <a.hahn@f5.com>
Adds a GitHub Actions workflow that builds and releases unitctl binaries
when a tag prefixed with `unitctl/` is pushed.
Binaries are built on pull-requests that change any files within
`tools/unitctl`, on `master` branch pushes and when `unitctl/` prefixed
tags are pushed.
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>
* use path seperator constant from rust std package
* pass a ControlSocket into deploy_new_container instead of a string
* parse and validate a ControlSocket from argument to instances new
* conditionally mount control socket only if its a unix socket
* use create_image in a way that actually pulls nonpresent images
* possibly override container command if TCP socket passed in
* handle more weird error cases
* add a ton of validation cases in the CLI command handler
* add a nice little progress bar :)
Signed-off-by: Ava Hahn <a.hahn@f5.com>
* add UnitdDockerError type
* write complete procedure to deploy unit via docker
* additional tweaks verifying it fails peacefully
* print important information in client
Signed-off-by: Ava Hahn <a.hahn@f5.com>
* refactored "instance" command out of enum
* plumbed through function stub from client library
* error handling
Signed-off-by: Ava Hahn <a.hahn@f5.com>
* move UnitdProcess serialization logic into UnitdProcess
* filter out docker processes from process output on Linux
* initial implementation of a UnitdContainer type
* initial implementation of a docker container search for unitd
* pull out custom openapi future executor and use same tokio
runtime as docker client
* refactor openapi client to not manage its own tokio runtime
* process mount points per docker container
* correctly output docker container info in relevant unitd
instances
* create UnitdProcess from UnitdContainer
* UnitdProcess now owns UnitdContainer
* get and parse container details from docker API
* introduce procedure to rewrite file paths based on docker
container mounts
* test path rewrite facilities
* apply path rewrite to unix socket
Signed-off-by: Ava Hahn <a.hahn@f5.com>
When we added -fno-strict-overflow to the CFLAGS back in c1e3f02f9
("Compile with -fno-strict-overflow") we inadvertently broke building
the Perl language module with clang, e.g
$ make
CC build/src/perl/nxt_perl_psgi-perl.o
clang: error: argument unused during compilation: '-fno-strict-overflow' [-Werror,-Wunused-command-line-argument]
This is due to for example on Apline
$ perl -MExtUtils::Embed -e ccflags
-D_REENTRANT -D_GNU_SOURCE -D_GNU_SOURCE -fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64
Where on clang the -fwrapv causes the -fno-strict-overflow to be
discarded resulting in the above error.
We can get around that by simply appending -Qunused-arguments to the
Perl CFLAGS.
This fixes things for _some_ systems, as there is actually another issue
with building this with clang on Fedora (and probably Red Hat) in that
there the Perl ccflags & ldopts have been heavily modified and uses
flags simply not only not in clang (which we can work around as above)
but also incompatible flags, e.g
$ make perl
CC build/src/perl/nxt_perl_psgi-perl.o
clang: error: optimization flag '-ffat-lto-objects' is not supported [-Werror,-Wignored-optimization-argument]
There doesn't seem to be an easy workaround like -Qunused-arguments for
this.
While we could work around it in some way, I'm not sure it's worth the
effort right now. On Red Hat & Fedora GCC _is_ the system compiler.
This could be revisited if we find people trying to build this on
Red Hat/Fedora with clang...
For comparison this is the Alpine Perl ccflags & ldops
$ perl -MExtUtils::Embed -e ccflags
-D_REENTRANT -D_GNU_SOURCE -D_GNU_SOURCE -fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 alpine:~$
$ perl -MExtUtils::Embed -e ldopts
-rdynamic -Wl,-rpath,/usr/lib/perl5/core_perl/CORE -fstack-protector-strong -L/usr/local/lib -L/usr/lib/perl5/core_perl/CORE -lperl -lpthread -ldl -lm -lcrypt -lutil -lc
Fedora
$ perl -MExtUtils::Embed -e ccflags
-D_REENTRANT -D_GNU_SOURCE -O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Wno-complain-wrong-lang -Werror=format-security -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -fwrapv -fno-strict-aliasing -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64
$ perl -MExtUtils::Embed -e ldopts
-Wl,--enable-new-dtags -Wl,-z,relro -Wl,--as-needed -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -Wl,--build-id=sha1 -Wl,-z,relro -Wl,--as-needed -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -Wl,--build-id=sha1 -fstack-protector-strong -L/usr/local/lib -L/usr/lib64/perl5/CORE -lperl -lpthread -lresolv -ldl -lm -lcrypt -lutil -lc
Fixes: c1e3f02f9 ("Compile with -fno-strict-overflow")
Signed-off-by: Andrew Clayton <a.clayton@nginx.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>
Since commit 0b5223e1c ("Disable strict-aliasing in clang by default")
we explicitly always build with -fno-strict-aliasing so there's no need
to set it independently in auto/modules/wasm
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
If it fails you can check the 'git log --check' output of the workflow
to see what the issue is. E.g
--- 93ec0133 Oops...
README.md:1: trailing whitespace.
+# NGINX Unit
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Co-developed-by: Ava Hahn <a.hahn@f5.com>
Signed-off-by: Ava Hahn <a.hahn@f5.com>
[ Tweak subject and cli => unitctl in README - Andrew ]
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
* Pull in entire unit-rust-sdk project
* not included: CLA, COC, License
* not included: duplicate openapi spec
* not included: CI workflows
* not included: changelog tooling
* not included: commitsar tooling
* not included: OpenAPI Web UI feature
* update links in unitctl manpage
* remove IDE configuration from .gitignore
* rename Containerfile.debian to Dockerfile
* simplify call to uname
* keep Readmes and Makefiles to 80 character lines
* outline specifically how to build unitctl
for any desired target, and where to then
find the binary for use
* remove a section on the vision of the CLI
which was superfluous given the state of
completeness of the code and its use in
unit
* remove out of date feature proposals from readme
* makefile: do not run when Rustup is not present
* bump mio version to latest
* generate openapi client library on demand
* generate-openapi only runs when not present
* generate-openapi now a dependency of binary build targets
* deleted autogenerated code
* reverted readme and Cargo document to autogenerated state
* add additional build requirement to Readme
Co-developed-by: Elijah Zupancic <e.zupancic@f5.com>
Signed-off-by: Elijah Zupancic <e.zupancic@f5.com>
Signed-off-by: Ava Hahn <a.hahn@f5.com>
Reviewed-by: Andrew Clayton <a.clayton@nginx.com> # non rust stuff
[ tools/cli => tools/unitctl and subject tweak - Andrew ]
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This continues the patch series constifying various pointers in the
configuration sub-system.
This is done as a separate commit as it involved a _slightly_ more
invasive change in nxt_conf_get_string().
While it takes a value parameter that is never modified, simply making
it const results in
CC build/src/nxt_conf.o
src/nxt_conf.c: In function ‘nxt_conf_get_string’:
src/nxt_conf.c:170:20: error: assignment discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
170 | str->start = value->u.str.start;
| ^
due to the assignment operator. Making value const will allow for
numerous other constification and seeing as we are not modifying it,
seems worthwhile.
We can get around the warning by casting ->u.{str,string}.start
Reviewed-by: Zhidao HONG <z.hong@f5.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
A common pattern was to declare variables in functions like
static nxt_str_t ...
Not sure why static, as they were being treated more like string
literals, let's actually make them constants (qualifier wise).
Reviewed-by: Zhidao HONG <z.hong@f5.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
A common pattern was to declare variables in functions like
static nxt_str_t ...
Not sure why static, as they were being treated more like string
literals (and of course they are _not_ thread safe), let's actually make
them constants (qualifier wise).
This handles core code conversion.
Reviewed-by: Zhidao HONG <z.hong@f5.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Mark numerous function argument pointers as 'const' in the configuration
sub-system.
This also does the same with a few functions in
src/nxt_conf_validation.c that are required to accomplish the below,
attacking the rest is an exercise for another day...
While this is a worthwhile hardening exercise in its own right, the main
impetus for this is to 'constify' some local function variables which
are currently defined with 'static' storage class and turn them into
'static const', which will be done in a subsequent patch.
Reviewed-by: Zhidao HONG <z.hong@f5.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Fixes: a48fbc035 ("Add additional information to the README")
Reviewed-by: Ava Hahn <a.hahn@f5.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Bumps <https://github.com/rustls/rustls> from 0.21.10 to 0.21.11.
"This release corrects a denial-of-service condition in
rustls::ConnectionCommon::complete_io(), reachable via network input. If
a close_notify alert is received during a handshake, complete_io() did
not terminate. Callers which do not call complete_io() are not
affected."
The wasm-wasi-component language module is not effected by this as it
doesn't handle client connections, Unit does.
Link: Release notes <https://github.com/rustls/rustls/releases>
Link: Commits <https://github.com/rustls/rustls/compare/v/0.21.10...v/0.21.11>
Signed-off-by: dependabot[bot] <support@github.com>
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
[ Tweaked commit message/subject - Andrew ]
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
