This short option is not really supported. Probably it was just a typo.
Reviewed-by: Artem Konev <a.konev@f5.com>
Signed-off-by: Alejandro Colomar <alx@nginx.com>
Normally Unit responds to HTTP requests by including a header like
Server: Unit/1.30.0
however it can sometimes be beneficial to withhold the version
information and in this case just respond with
Server: Unit
This patch adds a new "settings.http" boolean option called
server_version, which defaults to true, in which case the full version
information is sent. However this can be set to false, e.g
"settings": {
"http": {
"server_version": false
}
},
in which case Unit responds without the version information as the
latter example above shows.
Link: <https://www.ietf.org/rfc/rfc9110.html#section-10.2.4>
Closes: <https://github.com/nginx/unit/issues/158>
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Split out the "Unit" name from the NXT_SERVER #define into its own
NXT_NAME #define, then make NXT_SERVER a combination of that and
NXT_VERSION.
This is required for a subsequent commit where we may want the server
name on its own.
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
In nxt_kqueue_poll() error is declared as a nxt_bool_t aka unsigned int
(on x86-64 anyway).
It is used both as a boolean and as the return storage for a bitwise AND
operation.
This has potential to go awry.
If nxt_bool_t was changed to be a u8 then we would have the following
issue
gcc12 -c -pipe -fPIC -fvisibility=hidden -O -W -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wmissing-prototypes -Werror -g -O2 -I src -I build -I/usr/local/include -o build/src/nxt_kqueue_engine.o -MMD -MF build/src/nxt_kqueue_engine.dep -MT build/src/nxt_kqueue_engine.o src/nxt_kqueue_engine.c
src/nxt_kqueue_engine.c: In function 'nxt_kqueue_poll':
src/nxt_kqueue_engine.c:728:17: error: overflow in conversion from 'int' to 'nxt_bool_t' {aka 'unsigned char'} changes value from '(int)kev->flags & 16384' to '0' [-Werror=overflow]
728 | error = (kev->flags & EV_ERROR);
| ^
cc1: all warnings being treated as errors
EV_ERROR has the value 16384, after the AND operation error holds 16384,
however this overflows and wraps around (64 times) exactly to 0.
With nxt_bool_t defined as a u32, we would have a similar issue if
EV_ERROR ever became UINT_MAX + 1 (or a multiple thereof)...
Rather than conflating the use of error, keep error as a boolean (it is
used further down the function) but do the AND operation inside the
if ().
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This removes a bunch of unused files that would have been touched by
subsequent commits that switch to using nxt_bool_t (AKA unit6_t) in
structures.
In auto/sources we have
NXT_LIB_SRC0=" \
src/nxt_buf_filter.c \
src/nxt_job_file.c \
src/nxt_stream_module.c \
src/nxt_stream_source.c \
src/nxt_upstream_source.c \
src/nxt_http_source.c \
src/nxt_fastcgi_source.c \
src/nxt_fastcgi_record_parse.c \
\
src/nxt_mem_pool_cleanup.h \
src/nxt_mem_pool_cleanup.c \
"
None of these seem to actually be used anywhere (other than within
themselves). That variable is _not_ referenced anywhere else.
Also remove the unused related header files: src/nxt_buf_filter.h,
src/nxt_fastcgi_source.h, src/nxt_http_source.h, src/nxt_job_file.h,
src/nxt_stream_source.h and src/nxt_upstream_source.h
Also, these files do not seem to be used, no mention under auto/ or build/
src/nxt_file_cache.c
src/nxt_cache.c
src/nxt_job_file_cache.c
src/nxt_cache.h is #included in src/nxt_main.h, but AFAICT is not
actually used.
With all the above removed
$ ./configure --openssl --debug --tests && make -j && make -j tests &&
make libnxt
all builds.
Buildbot passes.
NOTE: You may need to do a 'make clean' before the next build attempt.
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Instead of doing the same operation in each subcommand, do it once in
the parent.
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
Signed-off-by: Alejandro Colomar <alx@nginx.com>
Don't reconstruct a new string for the $request_line from the parsed
method, target, and HTTP version, but rather keep a pointer to the
original memory where the request line was received.
This will be necessary for implementing URI rewrites, since we want to
log the original request line, and not one constructed from the
rewritten target.
This implementation changes behavior (only for invalid requests) in the
following way:
Previous behavior was to log as many tokens from the request line as
were parsed validly, thus:
Request -> access log ; error log
"GET / HTTP/1.1" -> "GET / HTTP/1.1" OK ; =
"GET / HTTP/1.1" -> "GET / HTTP/1.1" [1] ; =
"GET / HTTP/2.1" -> "GET / HTTP/2.1" OK ; =
"GET / HTTP/1." -> "GET / HTTP/1." [2] ; "GET / HTTP/1. [null]"
"GET / food" -> "GET / food" [2] ; "GET / food [null]"
"GET / / HTTP/1.1" -> "GET / / HTTP/1.1" [2] ; =
"GET / / HTTP/1.1" -> "GET / / HTTP/1.1" [2] ; =
"GET food HTTP/1.1" -> "GET" ; "GET [null] [null]"
"OPTIONS * HTTP/1.1" -> "OPTIONS" [3] ; "OPTIONS [null] [null]"
"FOOBAR baz HTTP/1.1"-> "FOOBAR" ; "FOOBAR [null] [null]"
"FOOBAR / HTTP/1.1" -> "FOOBAR / HTTP/1.1" ; =
"get / HTTP/1.1" -> "-" ; " [null] [null]"
"" -> "-" ; " [null] [null]"
This behavior was rather inconsistent. We have several options to go
forward with this patch:
- NGINX behavior.
Log the entire request line, up to '\r' | '\n', even if it was
invalid.
This is the most informative alternative. However, RFC-complying
requests will probably not send invalid requests.
This information would be interesting to users where debugging
requests constructed manually via netcat(1) or a similar tool, or
maybe for debugging a client, are important. It might be interesting
to support this in the future if our users are interested; for now,
since this approach requires looping over invalid requests twice,
that's an overhead that we better avoid.
- Previous Unit behavior
This is relatively fast (almost as fast as the next alternative, the
one we chose), but the implementation is ugly, in that we need to
perform the same operation in many places around the code.
If we want performance, probably the next alternative is better; if
we want to be informative, then the first one is better (maybe in
combination with the third one too).
- Chosen behavior
Only logging request lines when the request is valid. For any
invalid request, or even unsupported ones, the request line will be
logged as "-". Thus:
Request -> access log [4]
"GET / HTTP/1.1" -> "GET / HTTP/1.1" OK
"GET / HTTP/1.1" -> "GET / HTTP/1.1" [1]
"GET / HTTP/2.1" -> "-" [3]
"GET / HTTP/1." -> "-"
"GET / food" -> "-"
"GET / / HTTP/1.1" -> "GET / / HTTP/1.1" [2]
"GET / / HTTP/1.1" -> "GET / / HTTP/1.1" [2]
"GET food HTTP/1.1" -> "-"
"OPTIONS * HTTP/1.1" -> "-"
"FOOBAR baz HTTP/1.1"-> "-"
"FOOBAR / HTTP/1.1" -> "FOOBAR / HTTP/1.1"
"get / HTTP/1.1" -> "-"
"" -> "-"
This is less informative than previous behavior, but considering how
inconsistent it was, and that RFC-complying agents will probably not
send us such requests, we're ready to lose that information in the
log. This is of course the fastest and simplest implementation we
can get.
We've chosen to implement this alternative in this patch. Since we
modified the behavior, this patch also changes the affected tests.
[1]: Multiple successive spaces as a token delimiter is allowed by the
RFC, but it is discouraged, and considered a security risk. It is
currently supported by Unit, but we will probably drop support for
it in the future.
[2]: Unit currently supports spaces in the request-target. This is
a violation of the relevant RFC (linked below), and will be fixed
in the future, and consider those targets as invalid, returning
a 400 (Bad Request), and thus the log lines with the previous
inconsistent behavior would be changed.
[3]: Not yet supported.
[4]: In the error log, regarding the "log_routes" conditional logging
of the request line, we only need to log the request line if it
was valid. It doesn't make sense to log "" or "-" in case that
the request was invalid, since this is only useful for
understanding decisions of the router. In this case, the access
log is more appropriate, which shows that the request was invalid,
and a 400 was returned. When the request line is valid, it is
printed in the error log exactly as in the access log.
Link: <https://datatracker.ietf.org/doc/html/rfc9112#section-3>
Suggested-by: Liam Crilly <liam@nginx.com>
Reviewed-by: Zhidao Hong <z.hong@f5.com>
Cc: Timo Stark <t.stark@nginx.com>
Cc: Andrei Zeliankou <zelenkov@nginx.com>
Cc: Andrew Clayton <a.clayton@nginx.com>
Cc: Artem Konev <a.konev@f5.com>
Signed-off-by: Alejandro Colomar <alx@nginx.com>
Currently when running in the foreground, unit application processes
will send stdout to the current TTY and stderr to the unit log file.
That behaviour won't change.
When running as a daemon, unit application processes will send stdout to
/dev/null and stderr to the unit log file.
This commit allows to alter the latter case of unit running as a daemon,
by allowing applications to redirect stdout and/or stderr to specific
log files. This is done via two new application options, 'stdout' &
'stderr', e.g
"applications": {
"myapp": {
...
"stdout": "/path/to/log/unit/app/stdout.log",
"stderr": "/path/to/log/unit/app/stderr.log"
}
}
These log files are created by the application processes themselves and
thus the log directories need to be writable by the user (and or group)
of the application processes.
E.g
$ sudo mkdir -p /path/to/log/unit/app
$ sudo chown APP_USER /path/to/log/unit/app
These need to be setup before starting unit with the above config.
Currently these log files do not participate in log-file rotation
(SIGUSR1), that may change in a future commit. In the meantime these
logs can be rotated using the traditional copy/truncate method.
NOTE:
You may or may not see stuff printed to stdout as stdout was
traditionally used by CGI applications to communicate with the
webserver.
Closes: <https://github.com/nginx/unit/issues/197>
Closes: <https://github.com/nginx/unit/issues/846>
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This is analogous to the nxt_file_stderr() function and will be used in
a subsequent commit.
This function redirects stdout to a given file descriptor.
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
On GitHub, @jamesRUS52 reported that the PHP filter_input()[0] function
would just return NULL.
To enable this function we need to run the variables through the
sapi_module.input_filter() function when we call
php_register_variable_safe().
In PHP versions prior to 7.0.0, input_filter() takes 'len' as an
unsigned int, while later versions take it as a size_t.
Now, with this commit and the following PHP
<?php
var_dump(filter_input(INPUT_SERVER, 'REMOTE_ADDR'));
var_dump(filter_input(INPUT_SERVER, 'REQUEST_URI'));
var_dump(filter_input(INPUT_GET, 'get', FILTER_SANITIZE_SPECIAL_CHARS));
?>
you get
$ curl 'http://localhost:8080/854.php?get=foo<>'
string(3) "::1"
string(18) "/854.php?get=foo<>"
string(13) "foo<>"
[0]: <https://www.php.net/manual/en/function.filter-input.php>
Tested-by: <https://github.com/jamesRUS52>
Closes: <https://github.com/nginx/unit/issues/854>
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This allows us to be consistent through possible updates of default
settings used in distributions. Previous behaviour was uid/gid were
chosen automatically based on what uids/gids are already taken on the
system.
This will ensure we're checking out source code that is close to what we
have in binary packages.
While at it, remove the checkout directory when it's no longer needed.
If unitd was started with an explicit path then unitc will use that
binary instead of the default PATH to obtain the default control socket
and log file locations.
This was reported by the 'Clang Static Analyzer' as a 'dead nested
assignment'.
We assign prev_size then check if it's != 0 and if true we then set
prev_pages to page_size right shifted by two at the same time setting
prev_size to be right shifted by two (>>=), however page_size is never
used again so no need to set it here.
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
We allocate 'job' we then have a check if it's not NULL and do stuff
with it, but then we accessed it outside this check.
Simply return if job is NULL.
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This was reported by the 'Clang Static Analyzer' as a 'dead nested
assignment'.
We set end outside the loop but the first time we use it is to assign it
in the loop (not used anywhere else).
Further cleanup could be to reduce the scope of end by moving its
declaration inside the loop.
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This makes the build tree more organized, which is good for adding new
stuff. Now, it's useful for example for adding manual pages in man3/,
but it may be useful in the future for example for extending the build
system to run linters (e.g., clang-tidy(1), Clang analyzer, ...) on the
C source code.
Previously, the build tree was quite flat, and looked like this (after
`./configure && make`):
$ tree -I src build
build
├── Makefile
├── autoconf.data
├── autoconf.err
├── echo
├── libnxt.a
├── nxt_auto_config.h
├── nxt_version.h
├── unitd
└── unitd.8
1 directory, 9 files
And after this patch, it looks like this:
$ tree -I src build
build
├── Makefile
├── autoconf.data
├── autoconf.err
├── bin
│ └── echo
├── include
│ ├── nxt_auto_config.h
│ └── nxt_version.h
├── lib
│ ├── libnxt.a
│ └── unit
│ └── modules
├── sbin
│ └── unitd
├── share
│ └── man
│ └── man8
│ └── unitd.8
└── var
├── lib
│ └── unit
├── log
│ └── unit
└── run
└── unit
17 directories, 9 files
It also solves one issue introduced in
5a37171f73 ("Added default values for pathnames."). Before that
commit, it was possible to run unitd from the build system
(`./build/unitd`). Now, since it expects files in a very specific
location, that has been broken. By having a directory structure that
mirrors the installation, it's possible to trick it to believe it's
installed, and run it from there:
$ ./configure --prefix=./build
$ make
$ ./build/sbin/unitd
Fixes: 5a37171f73 ("Added default values for pathnames.")
Reported-by: Liam Crilly <liam@nginx.com>
Reviewed-by: Konstantin Pavlov <thresh@nginx.com>
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
Cc: Andrei Zeliankou <zelenkov@nginx.com>
Cc: Zhidao Hong <z.hong@f5.com>
Signed-off-by: Alejandro Colomar <alx@nginx.com>
In BSD systems, it's usually </var/db> or some other dir under </var>
that is not </var/lib>, so $statedir is a more generic name. See
hier(7).
Reported-by: Andrei Zeliankou <zelenkov@nginx.com>
Reported-by: Zhidao Hong <z.hong@f5.com>
Reviewed-by: Konstantin Pavlov <thresh@nginx.com>
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
Cc: Liam Crilly <liam@nginx.com>
Signed-off-by: Alejandro Colomar <alx@nginx.com>
We install jars with names like websocket-api-${NXT_JAVA_MODULE}-$NXT_VERSION.jar,
which translates to versioned NXT_JAVA_MODULE in the packaging system, e.g.
websocket-api-java11-1.30.0.jar.
When running as a daemon. unit currently sets umask(0), i.e no umask.
This is resulting in various directories being created with a mode of
0777, e.g
rwxrwxrwx
this is currently affecting cgroup and rootfs directories, which are
being created with a mode of 0777, and when running as a daemon as there
is no umask to restrict the permissions.
This also affects the language modules (the umask is inherited over
fork(2)) whereby unless something explicitly sets a umask, files and
directories will be created with full permissions, 0666 (rw-rw-rw-)/
0777 (rwxrwxrwx) respectively.
This could be an unwitting security issue.
My original idea was to just remove the umask(0) call and thus inherit
the umask from the executing shell/program.
However there was some concern about just inheriting whatever umask was
in effect.
Alex suggested that rather than simply removing the umask(0) call we
change it to a value of 022 (which is a common default), which will
result in directories and files with permissions at most of 0755
(rwxr-xr-x) & 0644 (rw-r--r--).
If applications need some other umask set, they can (as they always have
been able to) set their own umask(2).
Suggested-by: Alejandro Colomar <alx.manpages@gmail.com>
Reviewed-by: Liam Crilly <liam@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
When using the 'rootfs' isolation option, by default a tmpfs filesystem
is mounted on tmp/. Currently this is mounted with a mode of 0777, i.e
drwxrwxrwx. 3 root root 60 Feb 22 11:56 tmp
however this should really have the sticky bit[0] set (as is per-normal for
such directories) to prevent users from having free reign on the files
contained within.
What we really want is it mounted with a mode of 01777, i.e
drwxrwxrwt. 3 root root 60 Feb 22 11:57 tmp
[0]: To quote inode(7)
"The sticky bit (S_ISVTX) on a directory means that a file in that
directory can be renamed or deleted only by the owner of the file, by
the owner of the directory, and by a privileged process."
Reviewed-by: Liam Crilly <liam@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
