| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
Co-authored-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
This adds two helper function that will be used in subsequent commits.
nxt_http_comp_compress() does the actual compression.
nxt_http_comp_bound() returns the maximum compressed size for the given
size.
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
|
|
This exposes a new "settings.http.compression" configuration object.
Under which are types & compressors objects.
types is used to specify what MIME types should be considered
compressible.
compressors is used to configure an array of compressors that are
available. For each of these, you specify the encoding, e.g gzip and
optional level and min_length parameters. Where level is what
compression level to use and min_length is the minimum length of data
that should be compressed.
By default the default compression level for the specified compressor is
used and there is no minimum data length considered for compression.
It may look something like
"settings": {
"http": {
"server_version": true,
"static": {
"mime_types": {
"text/x-c": [
".c",
".h"
]
}
},
"compression": {
"types": [
"text/*"
],
"compressors": [
{
"encoding": "gzip",
"level": 3,
"min_length": 2048
},
{
"encoding": "deflate",
"min_length": 1024
},
{
"encoding": "zstd",
"min_length": 2048
},
{
"encoding": "br",
"min_length": 256
}
]
}
}
},
Currently this is a global option that will effect both static and
application responses.
In future it should be possible to add per-application (and perhaps even
per-static) configuration.
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
This allows to actually build unit with support fro zlib, zstd and
brotli compression.
Any or all can be specified. E.g.
$ ./configure --zlib ...
$ ./configure --zlib --zstd --brotli ...
During configure you will see if support for the requested compressions
has been found and what version of the library is being used.
E.g.
...
checking for zlib ... found
+ zlib version: 1.3.1.zlib-ng
checking for zstd ... found
+ zstd version: 1.5.6
checking for brotli ... found
+ brotli version: 1.1.0
...
Unit configuration summary:
...
zlib support: .............. YES
zstd support: .............. YES
brotli support: ............ YES
...
Co-authored-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
This adds support for both deflate & gzip compressors.
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
This is the initial step to enabling HTTP compression on both static and
application responses.
This code itself doesn't do any actual compression, that will come in
subsequent commits. It just contains the core functions for initialising
structures that describe the available compressors and functions for
checking if compression should be done depending on various criteria.
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
This is to store the MIME type of the response which will be used by the
HTTP compression patches as part of determining whether or not to
compress the response.
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
nxt_cpymem() is basically mempcpy(3)
Like mempcpy() nxt_cpymem() returns a void *.
nxt_cpymem() is implemented as a wrapper around memcpy(3), however
before returning the new pointer value we cast the return of memcpy(3)
to a u_char *, then add the length parameter to it.
I guess this was done to support compilers that do not support
arithmetic on void pointers as the C standard forbids it.
However since we removed support for compilers other than GCC and Clang
(ending in commit 9cd11133 ("Remove support for Sun's Sun Studio/SunPro
C compiler")) this is no longer an issue as both GCC and Clang support
arithmetic on void pointers (without the -pedantic option) by treating
the size of a void as 1.
While removing the unnecessary casting in this case doesn't necessarily
improve type-safety (as we're dealing with void *'s in and out), it does
just make the code that little more readable.
Oh and for interest we have actually already been relying on this
extension
src/nxt_array.c:143:40: warning: arithmetic on a pointer to void is a GNU extension [-Wgnu-pointer-arith]
143 | nxt_memcpy(data, src->elts + (i * size), size);
| ~~~~~~~~~ ^
src/nxt_string.h:45:24: note: expanded from macro 'nxt_memcpy'
45 | (void) memcpy(dst, src, length)
| ^~~
which was introduced in e2b53e16 ("Added "rootfs" feature.") back in
2020.
Link: <https://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html>
Link: <https://clang.llvm.org/docs/LanguageExtensions.html#introduction>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
|
|
Allow format to be an object to generate JSON logs. The object keys
become JSON field names, and values support string, variable, and JS.
Note that when there is no JS in the format values, the object will
be pre-serialized to a JSON template string at configuration phase
for better performance.
Example config:
{
"access_log": {
"path": "/tmp/access.log",
"format": {
"remote_addr": "$remote_addr",
"time_local": "$time_local",
"request_line": "$request_line",
"status": "$status",
"body_bytes_sent": "$body_bytes_sent",
"header_referer": "$header_referer",
"header_user_agent": "$header_user_agent"
}
}
}
|
|
This is a preparatory refactoring for upcoming JSON format support
in access log. We will extend format option to access object for
JSON support.
No functional changes.
|
|
This is a preparatory refactoring for upcoming JSON format support
in access log.
No functional changes.
|
|
This is a preparatory refactoring for upcoming JSON format support
in access log.
No functional changes.
|
|
These changes are generated by the openapi generator through a make
command.
Signed-off-by: Ava Hahn <a.hahn@f5.com>
Signed-off-by: Gabor Javorszky <g.javorszky@f5.com>
|
|
Tiny bracket balance fix.
Signed-off-by: Ava Hahn <a.hahn@f5.com>
Signed-off-by: Gabor Javorszky <g.javorszky@f5.com>
|
|
Adds code responsible for users to apply the `telemetry` configuration
options.
configuration snippet as follows:
{
"settings": {
"telemetry": {
"batch_size": 20,
"endpoint": "http://lgtm:4318/v1/traces",
"protocol": "http",
"sampling_ratio": 1
}
},
"listeners": {
"*:80": {
"pass": "routes"
}
},
"routes": [
{
"match": {
"headers": {
"accept": "*text/html*"
}
},
"action": {
"share": "/usr/share/unit/welcome/welcome.html"
}
},
{
"action": {
"share": "/usr/share/unit/welcome/welcome.md"
}
}
]
}
Signed-off-by: Ava Hahn <a.hahn@f5.com>
Signed-off-by: Gabor Javorszky <g.javorszky@f5.com>
|
|
Enables Unit to parse the tracestate and traceparent headers and add it
to the list, as well as calls to nxt_otel_test_and_call_state.
Signed-off-by: Ava Hahn <a.hahn@f5.com>
Signed-off-by: Gabor Javorszky <g.javorszky@f5.com>
|
|
Adds the --otel flag to the configure command and the various build time
variables and checks that are needed in this flow.
It also includes the nxt_otel.c and nxt_otel.h files that are needed for
the rest of Unit to talk to the compiled static library that's generated
from the rust crate.
Signed-off-by: Ava Hahn <a.hahn@f5.com>
Co-authored-by: Gabor Javorszky <g.javorszky@f5.com>
Signed-off-by: Gabor Javorszky <g.javorszky@f5.com>
|
|
This is purely the source code of the rust end of opentelemetry. It does
not have build tooling wired up yet, nor is this used from the C code.
Signed-off-by: Ava Hahn <a.hahn@f5.com>
Signed-off-by: Gabor Javorszky <g.javorszky@f5.com>
|
|
[ Tweaked subject - Andrew ]
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
This fixes an issue we had with wasm-wasi-component failing to load
components with
2024/11/06 21:08:50 [alert] 107196#107196 failed to create initial state
Caused by:
0: failed to compile component
1: WebAssembly translation error
2: Invalid input WebAssembly code at offset 15936: zero byte expected
Which was a symptom of
<https://github.com/bytecodealliance/wasmtime/issues/9130>
Closes: https://github.com/nginx/unit/issues/1477
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
When configuring under Linux we always got the following
checking for pthread spinlock zero initial value ... found but is not working
Having *actually* taken a look at this, this check seems somewhat bogus,
the first thing it does is
pthread_spinlock_t lock = 0;
which you shouldn't do anyway, you should use pthread_spin_init(3) to
initialise the pthread_spinlock_t variable.
But in any case, this thing, NXT_HAVE_PTHREAD_SPINLOCK_ZERO, isn't even
checked for in the code.
Neither is NXT_HAVE_PTHREAD_SPINLOCK, we don't use the pthread_spin_*
API, but rather roll our own spinlock implementation.
So let's just remove these checks, at the very least it'll speed
./configure up!
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
This does compile-time type and argument checking using a clang-plugin.
It was run as part of buildbot.
This covers unitd, src/test and the php, perl, python, ruby, wasm, java
and nodejs language modules/support.
It doesn't cover Go as that doesn't build anything with clang (uses
cgo) or wasm-wasi-component as that uses rustc.
Link: <https://github.com/nginx/clang-ast/tree/unit>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
nxt_log_alert() was missing the nxt_str_t parameter as required by the
%V format specifier.
This was found with the Unit clang-ast plugin.
Fixes: 7bf625394 ("Custom implementation of Base64 decoding function.")
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
sizeof() should never be used to get the size of an array. It is
very unsafe, since arrays easily decay to pointers, and sizeof()
applied to a pointer gives false results that compile and produce
silent bugs.
It's better to use nxt_items(), which implements sizeof()
division, which recent compilers warn when used with pointers.
This change would have caught a couple of bugs that were *almost*
introduced
First up is the _infamous_ ternary macro bug (yes, using the ternary
operator in a macro is of itself a bad idea)
nxt_str_set(&port, (r->tls ? "https://" : "http://"));
which in the macro expansion runs:
(&port)->length = nxt_length((r->tls ? : "https://" : "http://"));
which evaluates to:
port.length = sizeof(r->tls ? "https://" : "http://") - 1;
which evaluates to:
port.length = 8 - 1;
Of course, we didn't want a compile-time-constant 8 there, but
rather the length of the string.
The above bug is not obvious to the untrained eye, so let's show some
example programs that may give some more hints about the problem.
$ cat sizeof.c
#include <stdio.h>
int
main(void)
{
printf("%zu\n", sizeof("01"));
printf("%zu\n", sizeof("012"));
printf("%zu\n", sizeof(char *));
}
$ cc -Wall -Wextra sizeof.c
$ ./a.out
3
4
8
sizeof() returns the size in bytes of the array passed to it, which in
case of char strings, it is equivalent to the length of the string + 1
(for the terminating '\0').
However, arrays decay very easily in C, and they decay to a pointer to
the first element in the array. In case of strings, that is a 'char *'.
When sizeof() is given a pointer, it returns the size of the pointer,
which in most platforms is 8.
The ternary operator (?) performs default promotions (and other
nefarious stuff) that may surprise even the most experienced
programmers. It contrasts the __builtin_choose_expr() GCC builtin [1],
which performs almost equivalently, but without the unwanted effects of
the ternary operator.
[1]: <https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html#index-_005f_005fbuiltin_005fchoose_005fexpr>
$ cat ?.c
#include <stdio.h>
int
main(void)
{
printf("%zu\n", sizeof("01"));
printf("%zu\n", sizeof(__builtin_choose_expr(1, "01", "01")));
printf("%zu\n", sizeof(1 ? "01" : "01"));
printf("%zu\n", sizeof(char *));
}
$ cc -Wall -Wextra ?.c
$ ./a.out
3
3
8
8
In the above program, we can see how the ternary operator (?) decays
the array into a pointer, and makes it so that sizeof() will return a
constant 8.
As we can see, everything in the use of the macro would make it look
like it should work, but the combination of some seemingly-safe side
effects of various C features produces a completely unexpected bug.
Second up is a more straight forward case of simply calling nxt_length()
on a char * pointer.
Like the above this will generally result in a length of 7.
When you sit and think about it, you know very well sizeof(char *) is
probably 8 these days (but may be some other value like 4).
But when you're in the depths of code it's very easy to overlook this
when all you're thinking about is to get the length of some string.
Let's look at this patch in action
$ cat sdiv.c
#include <stdio.h>
#define nxt_nitems(x) (sizeof(x) / sizeof((x)[0]))
#define nxt_length(s) (nxt_nitems(s) - 1)
#define nxt_unsafe_length(s) (sizeof(s) - 1)
#define STR_LITERAL "1234567890"
static const char *str_lit = "1234567890";
int main(void)
{
printf("[STR_LITERAL] nxt_unsafe_length(\"1234567890\") [%lu]\n",
nxt_unsafe_length(STR_LITERAL));
printf("[STR_LITERAL] nxt_length(\"1234567890\") [%lu]\n",
nxt_length(STR_LITERAL));
printf("[char * ] nxt_unsafe_length(\"1234567890\") [%lu]\n",
nxt_unsafe_length(str_lit));
printf("[char * ] nxt_length(\"1234567890\") [%lu]\n",
nxt_length(str_lit));
return 0;
}
First lets compile it without any flags
$ make sdiv
$ ./sdiv
[STR_LITERAL] nxt_unsafe_length("1234567890") [10]
[STR_LITERAL] nxt_length("1234567890") [10]
[char * ] nxt_unsafe_length("1234567890") [7]
[char * ] nxt_length("1234567890") [7]
It compiled without error and runs, although with incorrect results for
the two char *'s.
Now lets build it with -Wsizeof-pointer-div (also enabled with -Wall)
$ CFLAGS="-Wsizeof-pointer-div" make sdiv
cc -Wsizeof-pointer-div nxt_nitems.c -o nxt_nitems
sdiv.c: In function ‘main’:
sdiv.c:3:44: warning: division ‘sizeof (const char *) / sizeof (char)’ does not compute the number of array elements [-Wsizeof-pointer-div]
3 | #define nxt_nitems(x) (sizeof(x) / sizeof((x)[0]))
| ^
nxt_nitems.c:4:34: note: in expansion of macro ‘nxt_nitems’
4 | #define nxt_length(s) (nxt_nitems(s) - 1)
| ^~~~~~~~~~
nxt_nitems.c:22:16: note: in expansion of macro ‘nxt_length’
22 | nxt_length(str_lit));
| ^~~~~~~~~~
nxt_nitems.c:10:20: note: first ‘sizeof’ operand was declared here
10 | static const char *str_lit = "1234567890";
| ^~~~~~~
So we now get a very loud compiler warning (coming from nxt_length(char
*), nxt_unsafe_length() of course didn't trigger any warnings), telling
us we're being daft.
The good news is this didn't find any existing bugs! Let's keep it that
way...
Link: <https://stackoverflow.com/a/57537491>
Cc: Andrew Clayton <a.clayton@nginx.com>
Signed-off-by: Alejandro Colomar <alx@nginx.com>
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
Tested-by: Andrew Clayton <a.clayton@nginx.com>
[ Tweaked and expanded the commit message - Andrew ]
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
Mostly more 'static nxt_str_t ...'s
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
When using JS configuration for the "format" option, access log entries
were being written without newline characters. This commit adds the
missing newline character to each log entry.
Closes: https://github.com/nginx/unit/issues/1458
|
|
This commit introduces a new flag to control the addition of newline
characters in access log entries. This is prepared for fixing the issue
where log entries lack newlines when using JS configuration.
|
|
In the perl language module we create a new perl *module* on the fly
comprised of some preamble, the specified perl script and some
post-amble.
In the preamble we create a constructor called new(), however this can
clash with other constructors also called new.
While this can be worked around by instead of doing
... new CLASS
rather do
... CLASS->new()
While this constructor was added in commit 3b2c1d0e ("Perl: added
implementation delayed response and streaming body."), I don't see that
we actually use it anywhere (nor is it seemingly something we document)
and if we simply remove it then things still seem to work, including the
Perl pytests
...
test/test_perl_application.py::test_perl_streaming_body_multiple_responses[5.38.2] PASSED
...
test/test_perl_application.py::test_perl_delayed_response[5.38.2] PASSED
test/test_perl_application.py::test_perl_streaming_body[5.38.2] PASSED
...
Closes: https://github.com/nginx/unit/issues/1456
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
Under Ubuntu 24.04 the pytest for
test/test_php_isolation.py::test_php_isolation_rootfs fails due to Unit
aborting (SIGABRT) in the PHP language module due to FORIFY_SOURCE
hardening detecting a buffer overflow
2024/10/16 16:46:54 [info] 11661#11661 "phpinfo" application started
*** buffer overflow detected ***: terminated
2024/10/16 16:46:54 [alert] 11660#11660 app process 11661 exited on signal 6
After spending an extraordinary amount of time faffing around with
Ubuntu and pytests (they don't make for a pleasant combination) I was
able to reproduce it.
The crash was occurring here
#4 0x00007ebe818288ff in __GI_abort () at ./stdlib/abort.c:79
#5 0x00007ebe818297b6 in __libc_message_impl (
fmt=fmt@entry=0x7ebe819ce765 "*** %s ***: terminated\n")
at ../sysdeps/posix/libc_fatal.c:132
#6 0x00007ebe81936c19 in __GI___fortify_fail (
msg=msg@entry=0x7ebe819ce74c "buffer overflow detected")
at ./debug/fortify_fail.c:24
#7 0x00007ebe819365d4 in __GI___chk_fail () at ./debug/chk_fail.c:28
#8 0x00007ebe8134a055 in mempcpy (__len=10, __src=0x7ebe8160ade8,
__dest=0x571ba9bd0930)
at /usr/include/x86_64-linux-gnu/bits/string_fortified.h:45
#9 fake_data_segment (info=0x0, sysdb=0x571ba9bcf080)
at /usr/src/php8.1-8.1.30-1+ubuntu24.04.1+deb.sury.org+1/ext/date/lib/parse_tz.c:921
#10 timelib_builtin_db ()
at /usr/src/php8.1-8.1.30-1+ubuntu24.04.1+deb.sury.org+1/ext/date/lib/parse_tz.c:1084
#11 0x00007ebe812e0885 in zm_info_date (zend_module=0x571ba9a14420)
[Well as best as I can tell, as this is from the php 8.1 packages from
<https://github.com/oerdnj/deb.sury.org>, I don't know where the
packages (I'm assuming it's packages) shivammathur/setup-php@v2
installs come from.]
So we get killed in fake_data_segment(), the thing is, that function (as
well as timelib_builtin_db()) doesn't exist in upstream PHP.
It turns out these come from a patch that is applied by distributions to
make PHP use the system installed timezone database rather than the one
built into PHP.
I was unable to reproduce this with vanilla PHP 8.1.
It can be triggered on affected builds with the following config
{
"listeners": {
"[::1]:8080": {
"pass": "applications/php"
}
},
"applications": {
"php": {
"type": "php",
"root": "/app/php",
"isolation": {
"rootfs": "/tmp/unit-root",
"namespaces": {
"mount": true,
"credential": true,
"pid": true
}
}
}
}
}
The crux of the issue seems to come down to in this case PHP can't open
the tz database as it's not contained in the new mount namespace.
190437 openat(AT_FDCWD, "/usr/share/zoneinfo/", O_RDONLY|O_NONBLOCK|O_CLOEXEC|O_DIRECTORY) = -1 ENOENT (No such file or directory)
190437 openat(AT_FDCWD, "/usr/share/zoneinfo/zone.tab", O_RDONLY) = -1 ENOENT (No such file or directory)
190437 writev(2, [{iov_base="*** ", iov_len=4}, {iov_base="buffer overflow detected", iov_len=24}, {iov_base=" ***: terminated\n", iov_len=17}], 3) = 45
...
190437 --- SIGABRT {si_signo=SIGABRT, si_code=SI_TKILL, si_pid=2, si_uid=65534} ---
190437 +++ killed by SIGABRT +++
Specifically the issue is with the following code in the patch
(certainly an earlier version of the patch, this is from a Debian patch
<https://sources.debian.org/src/php8.2/8.2.20-1~deb12u1/debian/patches/0007-Add-support-for-use-of-the-system-timezone-database.patch/>)
+ data = malloc(3 * sysdb->index_size + 7);
+
+ p = mempcpy(data, FAKE_HEADER, sizeof(FAKE_HEADER) - 1);
If the zone file hasn't been found then sysdb->index_size is 0. So we
malloc(3) a total of 7 bytes.
However, sizeof(FAKE_HEADER) - 1 is 10. (Hence the __len=10 in the
mempcpy(3) in the above backtrace).
Of course 10 doesn't fit into 7 and the FORTIFY_SOURCE hardening kicks
in and SIGABRTs the process.
Now, it's worth noting that this issue doesn't occur with PHP 8.2 and
8.3.
As can been seen from the Fedora patch for this
<https://src.fedoraproject.org/rpms/php/blob/rawhide/f/php-8.4.0-systzdata-v24.patch>
They actually have a fix incorporated
r23: fix possible buffer overflow
So the above patch now does
+ data = malloc(3 * sysdb->index_size + sizeof(FAKE_HEADER) - 1);
+
+ p = mempcpy(data, FAKE_HEADER, sizeof(FAKE_HEADER) - 1);
So you will always get at least the required 10 bytes allocated.
I assume the PHP 8.2 & 8.3 packages either no longer use this patch or
have the fixed version. I don't know... I haven't found the sources...
Anyway the above was more about satisfying myself that the problem
wasn't with Unit.
PHP 8.1 is now in security maintenance mode and people are actively
encouraged to upgrade to 8.2/8.3
So lets just remove 8.1 from our testing...
[It's also worth noting that after all this, the ubuntu-latest runners
seemed to have switched back from 24.04 to 22.04. However lets stick
with this and the other ci fixes as who knows when it'll go back to
24.04 (or some other version) again...]
Link: <https://www.php.net/supported-versions.php>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
With Ubuntu 24.04 installing it via pip gave this error
error: externally-managed-environment
× This environment is externally managed
╰─> To install Python packages system-wide, try apt install
python3-xyz, where xyz is the package you are trying to
install.
If you wish to install a non-Debian-packaged Python package,
create a virtual environment using python3 -m venv path/to/venv.
Then use path/to/venv/bin/python and path/to/venv/bin/pip. Make
sure you have python3-full installed.
If you wish to install a non-Debian packaged Python application,
it may be easiest to use pipx install xyz, which will manage a
virtual environment for you. Make sure you have pipx installed.
See /usr/share/doc/python3.12/README.venv for more information.
Installing it via the package manager is the better option anyway...
Under Ubuntu 22.04 it only installs a /usr/bin/pytest-3 binary, rather
than installing a /usr/bin/pytest binary and symlink for pytest-3, so
use pytest-3 as the command.
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
With Ubuntu 24.04 this service is no longer enabled/installed and so
this bit would fail.
This commit makes it handle both cases (installed/not-installed).
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
Bumps <https://github.com/bytecodealliance/wasmtime> from 24.0.0 to
24.0.1.
Fixes:
a runtime crash when combining tail-calls with host imports that
capture a stack trace or trap. GHSA-q8hx-mm92-4wvg
a race condition could lead to WebAssembly control-flow integrity and
type safety violations. GHSA-7qmx-3fpx-r45m
Link: Release notes <https://github.com/bytecodealliance/wasmtime/releases>
Link: Changelog <https://github.com/bytecodealliance/wasmtime/blob/main/docs/WASI-some-possible-changes.md>
Link: Commits <https://github.com/bytecodealliance/wasmtime/compare/v24.0.0...v24.0.1>
Signed-off-by: dependabot[bot] <support@github.com>
[ Tweaked commit message/subject - Andrew ]
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
There has been a long standing clang-analyzer issue in
nxt_process_check_pid_status(), well ever since I introduced this
function in commit b0e2d9d0a ("Isolation: Switch to fork(2) & unshare(2)
on Linux."),
It is complaining that there are cases where 'status' could be returned
with an undefined or garbage value.
Now I'm convinced this can't happen
If nxt_process_pipe_timer() returns NXT_OK
read(2) the status value
If the read(2) failed or if we got NXT_ERROR from
nxt_process_pipe_timer(), NXT_OK (0) and NXT_ERROR (-1) are the only
possible return values here, then we set status to -1
I don't see a scenario where status is either not set by the read(2) or
not set to -1.
Now I'm not a fan of initialising variables willy-nilly, however, in
this case if we initialise status to -1, then we can simply remove the
if (ret <= 0) {
check. So it closes this (non-)issue but also provides a little code
simplification.
NOTE: There is no need to check the return from the read(2) here. We are
reading a single byte, we either get it or don't.
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
Signed-off-by: Ava Hahn <a.hahn@f5.com>
|
|
Signed-off-by: Ava Hahn <a.hahn@f5.com>
|
|
The function nxt_term_parse() is able to take strings with trailing
whitespace e.g. "1w1d ", add a test case to cover such things.
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
Both clang-analyzer and coverity flagged an issue in nxt_term_parse()
that we set 'state = st_letter' but then set it to 'state = st_space'
before using it.
While we could simply remove the first assignment and placate the
analyzers, upon further analysis it seems that there is some more
cleanup that could be done in this function.
This commit addresses the above issue, subsequent commits will continue
the cleanup.
To solve the unused assignment issue we can get rid of the
'state == st_letter'
assignment and unconditionally execute the code that was behind the
if (state != st_letter) {
guard. If we're not handling a space then we should have either a digit
or letter.
Also, perhaps more importantly, this if () statement would never be
false at this point as state would never == st_letter.
We may as well also remove the st_letter enum value.
The src/test/nxt_term_parse_test.c still passes
tests: [notice] term parse test passed
NOTE: Although this function is not currently used in Unit (only by
src/test/nxt_term_parse_test.c), it is probably worth cleaning it up and
solving one of the open clang-analyzer (and coverity) issues.
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
Due to 'char' (unless explicitly set) being signed or unsigned depending
on architecture, e.g on x86 it's signed, while on Arm it's unsigned,
this can lead to subtle bugs such if you use a plain char as a byte
thinking it's unsigned on all platforms (maybe you live in the world of
Arm).
What we can do is tell the compiler to treat 'char' as unsigned by
default, thus it will be consistent across platforms. Seeing as most of
the time it doesn't matter whether char is signed or unsigned, it
really only matters when you're dealing with 'bytes', which means it
makes sense to default char to unsigned.
The Linux Kernel made this change at the end of 2022.
This will also allow in the future to convert our u_char's to char's
(which will now be unsigned) and pass them directly into the libc
functions for example, without the need for casting.
Here is what the ISO C standard has to say
From §6.2.5 Types ¶15
The three types char, signed char, and unsigned char are collectively
called the character types. The implementation shall define char to
have the same range, representation, and behavior as either signed
char or unsigned char.[45]
and from Footnote 45)
CHAR_MIN, defined in <limits.h>, will have one of the values 0 or
SCHAR_MIN, and this can be used to distinguish the two options.
Irrespective of the choice made, char is a separate type from the
other two and is not compatible with either.
If you're still unsure why you'd want this change...
It was never clear to me, why we used u_char, perhaps that was used as
an alternative to -funsigned-char...
But that still leaves the potential for bugs with char being unsigned vs
signed...
Then because we use u_char but often need to pass such things into libc
(and perhaps other functions) which normally take a 'char' we need to
cast these cases.
So this change brings at least two (or more) benefits
1) Removal of potential for char unsigned vs signed bugs.
2) Removal of a bunch of casts. Reducing casting to the bare minimum
is good. This helps the compiler to do proper type checking.
3) Readability/maintainability, everything is now just char...
What if you want to work with bytes?
Well with char being unsigned (everywhere) you can of course use char.
However it would be much better to use the uint8_t type for that to
clearly signify that intention.
Link: <https://lore.kernel.org/lkml/Y1Bfg06qV0sDiugt@zx2c4.com/>
Link: <https://lore.kernel.org/lkml/20221019203034.3795710-1-Jason@zx2c4.com/>
Link: <https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3bc753c06dd02a3517c9b498e3846ebfc94ac3ee>
Link: <https://www.iso-9899.info/n1570.html#6.2.5p15>
Suggested-by: Alejandro Colomar <alx@kernel.org>
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
Better late than never!
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
This is autogenerated from docs/changes.xml by
$ make -C docs/ changes && mv build/CHANGES .
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
Signed-off-by: Ava Hahn <a.hahn@f5.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
