Both @lucatacconi & @mwoodpatrick reported what appears to be the same
issue on GitHub. Namely that when using the PHP language module and
trying to access a URL that is a directory but without specifying the
trailing '/', they were getting a '503 Service Unavailable' error.
Note: This is when _not_ using the 'script' option.
E.g with the following config
{
"listeners": {
"[::1]:8080": {
"pass": "applications/php"
}
},
"applications": {
"php": {
"type": "php",
"root": "/var/tmp/unit-php"
}
}
}
and with a directory path of /var/tmp/unit-php/foo containing an
index.php, you would see the following
$ curl http://localhost/foo
<title>Error 503</title>
Error 503
However
$ curl http://localhost/foo/
would work and serve up the index.php
This commit fixes the above so you get the desired behaviour without
specifying the trailing '/' by doing the following
1] If the URL doesn't end in .php and doesn't have a trailing '/'
then check if the requested path is a directory.
2) If it is a directory then create a 301 re-direct pointing to it.
This matches the behaviour of the likes of nginx, Apache and
lighttpd.
This also matches the behaviour of the "share" action in Unit.
This doesn't effect the behaviour of the 'script' option which bypasses
the nxt_php_dynamic_request() function.
This also adds a couple of tests to test/test_php_application.py to
ensure this continues to work.
Closes: <https://github.com/nginx/unit/issues/717>
Closes: <https://github.com/nginx/unit/issues/753>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Future releases of GCC will render function definitions like
func()
invalid by default. See the previous commit 09f88c9 ("Fixed main()
prototypes in auto tests.") for details.
Such functions should be defined like
func(void)
This is a good thing to do regardless of the upcoming GCC changes.
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Future releases of GCC are planning to remove[0] default support for
some old features that were removed from C99 but GCC still accepts.
We can test for these changes by using the following -Werror=
directives
-Werror=implicit-int
-Werror=implicit-function-declaration
-Werror=int-conversion
-Werror=strict-prototypes
-Werror=old-style-definition
Doing so revealed an issue with the auto/ tests in that the test
programs always define main as
int main()
rather than
int main(void)
which results in a bunch of errors like
build/autotest.c:3:23: error: function declaration isn't a prototype [-Werror=strict-prototypes]
3 | int main() {
| ^~~~
build/autotest.c: In function 'main':
build/autotest.c:3:23: error: old-style function definition [-Werror=old-style-definition]
The fix was easy, it only required fixing the main prototype with
find -type f -exec sed -i 's/int main() {/int main(void) {/g' {} \;
Regardless of these upcoming GCC changes, this is probably a good thing
to do anyway for correctness.
[0]: https://fedoraproject.org/wiki/Changes/PortingToModernC
Link: <https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/message/CJXKTLXJUPZ4F2C2VQOTNMEA5JAUPMBD/>
Link: <https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/message/6SGHPHPAXKCVJ6PUZ57WVDQ5TDBVIRMF/>
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Some distros provide it in /bin/sed and others in both /bin/sed
and /usr/bin/sed. Use the more available one.
Reported-by: Konstantin Pavlov <thresh@nginx.com>
Fixes: ac64ffde57 "Improved readability of <docker-entrypoint.sh>."
Signed-off-by: Alejandro Colomar <alx@nginx.com>
'install -d' has an issue compared to 'mkdir -p': it doesn't
respect existing directories. It will set the ownership, file
mode, and SELinux contexts (and any other property that would be
set by install(1) to a newly-created directory), overwriting any
existing properties of the existing directory.
'mkdir -p' doesn't have this issue: it is a no-op if the
directory exists. However, it's not an ideal solution either,
since it can't be used to set the properties (owner, mode, ...) of
a newly-created directory.
Therefore, the best solution is to use install(1), but only after
making sure that the directory doesn't exist with test(1).
Reported-by: Andrew Clayton <a.clayton@nginx.com>
Reported-by: Alejandro Colomar <alx@nginx.com>
Closes: <https://github.com/nginx/unit/issues/769>
Signed-off-by: Alejandro Colomar <alx@nginx.com>
Tested-by: Andrew Clayton <a.clayton@nginx.com>
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
This commit removed the $uri auto-append for the "share" option
introduced in rev be6409cdb028.
The main reason is that it causes problems when preparing Unit configurations
to be loaded at startup from the state directory. E.g. Docker. A valid conf.json
file with $uri references will end up with $uri$uri due to the auto-append.
pthread_mutex_init(3) may fail for several reasons, and failing to
check will cause Undefined Behavior when those errors happen. Add
missing checks, and correctly deinitialize previously created
stuff before exiting from the API.
Signed-off-by: Alejandro Colomar <alx@nginx.com>
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
Reviewed-by: Zhidao HONG <z.hong@f5.com>
@alejandro-colomar reported that the build was broken on MacOS
cc -o build/unitd -pipe -fPIC -fvisibility=hidden -O -W -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -fstrict-aliasing -Wstrict-overflow=5 -Wmissing-prototypes -Werror -g \
build/src/nxt_main.o build/libnxt.a \
\
\
-L/usr/local/Cellar/pcre2/10.40/lib -lpcre2-8
Undefined symbols for architecture x86_64:
"_nxt_fs_mkdir_parent", referenced from:
_nxt_runtime_pid_file_create in libnxt.a(nxt_runtime.o)
_nxt_runtime_controller_socket in libnxt.a(nxt_controller.o)
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [build/unitd] Error 1
This was due to commit 57fc920 ("Socket: Created control socket & pid file
directories.").
This happened because this commit introduced the usage of
nxt_fs_mkdir_parent() in core code which uses nxt_fs_mkdir(), both of
these are defined in src/nxt_fs.c. It turns out however that this file
doesn't get built on MacOS (or any system that isn't Linux or that
lacks a FreeBSD compatible nmount(2) system call) due to the following
In auto/sources we have
if [ $NXT_HAVE_ROOTFS = YES ]; then
NXT_LIB_SRCS="$NXT_LIB_SRCS src/nxt_fs.c"
fi
NXT_HAVE_ROOTFS is set in auto/isolation
If [ $NXT_HAVE_MOUNT = YES -a $NXT_HAVE_UNMOUNT = YES ]; then
NXT_HAVE_ROOTFS=YES
cat << END >> $NXT_AUTO_CONFIG_H
#ifndef NXT_HAVE_ISOLATION_ROOTFS
#define NXT_HAVE_ISOLATION_ROOTFS 1
#endif
END
fi
While we do have a check for a generic umount(2) which is found on
MacOS, for mount(2) we currently only check for the Linux mount(2) and
FreeBSD nmount(2) system calls. So NXT_HAVE_ROOTFS is set to NO on MacOS
and we don't build src/nxt_fs.c
This fixes the immediate build issue by taking the mount/umount OS
support out of nxt_fs.c into a new nxt_fs_mount.c file which is guarded
by the above while we now build nxt_fs.c unconditionally.
This should fix the build on any _supported_ system.
Reported-by: Alejandro Colomar <alx@nginx.com>
Fixes: 57fc920 ("Socket: Created control socket & pid file directories.")
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Access log used for the variables testing instead of limited routing.
Added missed test for $status variable.
Some tests moved from "test_access_log.py" to "test_variables.py".
Ruby applications would fail to start if they were using rack v3
2022/09/28 15:48:46 [alert] 0#80912 [unit] Ruby: Failed to parse rack script
2022/09/28 15:48:46 [notice] 80911#80911 app process 80912 exited with code 1
This was due to a change in the rack API
Rack V2
def self.load_file(path, opts = Server::Options.new)
...
cfgfile.sub!(/^__END__\n.*\Z/m, '')
app = new_from_string cfgfile, path
return app, options
end
Rack V3
def self.load_file(path)
...
return new_from_string(config, path)
end
This patch handles _both_ the above APIs by correctly handling the cases
where we do and don't get an array returned from
nxt_ruby_rack_parse_script().
Closes: <https://github.com/nginx/unit/issues/755>
Tested-by: Andrew Clayton <a.clayton@nginx.com>
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
[ Andrew: Patch by Zhidao, commit message by me with input from Zhidao ]
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This is a preparatory patch that renames the 'local' and 'local_length'
members of the nxt_unit_request_t structure to 'local_addr' and
'local_addr_length' in preparation for the adding of 'local_port' and
'local_port_length' members.
Suggested-by: Zhidao HONG <z.hong@f5.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
@alejandro-colomar reported an issue on GitHub whereby Unit would fail
to start due to not being able to create the control socket (a Unix
Domain Socket)
2022/08/05 20:12:22 [alert] 21613#21613 bind(6,
unix:/opt/local/unit/var/run/unit/control.unit.sock.tmp)
failed (2: No such file or directory)
This could happen if the control socket was set to a directory that
doesn't exist. A common place to put the control socket would be under
/run/unit, and while /run will exist, /run/unit may well not (/run
is/should be cleared on each boot).
The pid file would also generally go under /run/unit, though this is
created after the control socket, however it could go someplace else so
we should also ensure its directory exists.
This commit will try to create the pid file and control sockets parent
directory. In some cases the user will need to ensure that the rest of
the path already exists.
This adds a new nxt_fs_mkdir_parent() function that given a full path
to a file (or directory), strips the last component off before passing
the remaining directory path to nxt_fs_mkdir().
Cc: Konstantin Pavlov <thresh@nginx.com>
Closes: <https://github.com/nginx/unit/issues/742>
Reported-by: Alejandro Colomar <alx@nginx.com>
Reviewed-by: Alejandro Colomar <alx@nginx.com>
Tested-by: Alejandro Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This file is used by git (maybe also hg) to map different identities.
It can be used to map different names/email addresses to a preferred
name/email. So if you have authored/committed under different
names/email addresses you can map all these to a particular name/email.
Certain git commands (log, shortlog, show, blame etc) will consult this
file and show the Author/Committer accordingly.
Note: This does _not_ change history, this is simply used by various
commands to alter their output and can be disabled for some commands
with the --no-mailmap option.
This can be useful for commands like git shortlog so that all your
commits are shown under a single identity and also so people have an
up to date email address should they wish to contact you. And just for
overall consistency.
Seeing as I've already committed under two different email addresses
(not counting this one), I've put entries in this file to map my email
addresses to my @nginx.com one.
See also, gitmailmap(5).
Acked-by: Alex Colomar <alx@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
In nxt_unit_create() we could leak a mutex created in
nxt_unit_ctx_init().
This could happen if nxt_unit_ctx_init() succeeded but later on we
bailed out of nxt_unit_create(), we would destroy the mutex created in
nxt_unit_create() but not the one created in nxt_unit_ctx_init().
Reorder things so that we do the call to nxt_unit_create() after all the
other checks so if it fails we don't leak the mutex it created.
Co-developed-by: Andrew Clayton <a.clayton@f5.com>
Signed-off-by: Andrew Clayton <a.clayton@f5.com>
Signed-off-by: Alex Colomar <a.colomar@f5.com>
As was reported[0] by @travisbell on GitHub, if running unit from the
terminal in the foreground when hitting ^C to exit it, the ruby
application processes would segfault if they were using threads.
It's not 100% clear where the actual problem lies, but it _looks_ like
it may be in ruby.
The simplest way to deal with this for now is to just ignore SIGINT in
the ruby application processes. Unit will still receive and handle it,
cleanly shutting everything down.
For people who want to handle SIGINT in their ruby application running
under unit they can still trap SIGINT and it will override the ignore.
[0]: https://github.com/nginx/unit/issues/562#issuecomment-1223229585
Closes: https://github.com/nginx/unit/issues/562
The previous commit added/fixed support for abstract Unix domain sockets
on Linux with a leading '@' or '\0'. To be consistent in all platforms,
treat those prefixes as markers for abstract sockets in all platforms,
and fail if abstract sockets are not supported by the platform.
That will avoid mistakes when copying a config file from a Linux system
and using it in non-Linux, which would surprisingly create a normal socket.
We accept both "\u0000socket-name" and "@socket-name" as abstract
unix sockets. The first one is passed to the kernel pristine,
while the second is transformed '@'->'\0'.
The commit that added support for unix sockets accepts both
variants, but we internally stored it in the same way, using
"\u0000..." for both.
We want to support abstract sockets transparently to the user, so
that if the user configures unitd with '@', if we receive a query
about the current configuration, the user should see the same
exact thing that was configured. So, this commit avoids the
transformation in the internal state file, storing user input
pristine, and we only transform the '@' for a string that will
be used internally (not user-visible).
This commit (indirectly) fixes a small bug, where we created
abstract sockets with a trailing '\0' in their name due to calling
twice nxt_sockaddr_parse() on the same string. By calling that
function only once with each copy of the string, we have fixed that
bug.
Unix domain sockets are normally backed by files in the
filesystem. This has historically been problematic when closing
and opening again such sockets, since SO_REUSEADDR is ignored for
Unix sockets (POSIX left the behavior of SO_REUSEADDR as
implementation-defined, and most --if not all-- implementations
decided to just ignore this flag).
Many solutions are available for this problem, but all of them
have important caveats:
- unlink(2) the file when it's not needed anymore.
This is not easy, because the process that controls the fd may
not be the same process that created the file, and may not have
file permissions to remove it.
Further solutions can be applied to that caveat:
- unlink(2) the file right after creation.
This will remove the pathname from the filesystem without
closing the socket (it will continue to live until the last fd
is closed). This is not useful for us, since we need the
pathname of the socket as its interface.
- chown(2) or chmod(2) the directory that contains the socket.
For removing a file from the filesystem, a process needs
write permissions in the containing directory. We could
put sockets in dummy directories that can be chown(2)ed to
nobody. This could be dangerous, though, as we don't control
the socket names. It is our users who configure the socket
name in their configuration, and so it's easy that they don't
understand the many implications of not chosing an appropriate
socket pathname. A user could unknowingly put the socket in a
directory that is not supposed to be owned by user nobody, and
if we blindly chown(2) or chmod(2) the directory, we could be
creating a big security hole.
- Ask the main process to remove the socket.
This would require a very complex communication mechanism with
the main process, which is not impossible, but let's avoid it
if there are simpler solutions.
- Give the child process the CAP_DAC_OVERRIDE capability.
That is one of the most powerful capabilities. A process with
that capability can be considered root for most practical
aspects. Even if the capability is disabled for most of the
lifetime of the process, there's a slight chance that a
malicious actor could activate it and then easily do serious
damage to the system.
- unlink(2) the file right before calling bind(2).
This is dangerous because another process (for example, another
running instance of unitd(8)), could be using the socket, and
removing the pathname from the filesystem would be problematic.
To do this correctly, a lot of checks should be added before the
actual unlink(2), which is error-prone, and difficult to do
correctly, and atomically.
- Use abstract-namespace Unix domain sockets.
This is the simplest solution, as it only requires accepting a
slightly different syntax (basically a @ prefix) for the socket
name, to transform it into a string starting with a null byte
('\0') that the kernel can understand. The patch is minimal.
Since abstract sockets live in an abstract namespace, they don't
create files in the filesystem, so there's no need to remove
them later. The kernel removes the name when the last fd to it
has been closed.
One caveat is that only Linux currently supports this kind of
Unix sockets. Of course, a solution to that could be to ask
other kernels to implement such a feature.
Another caveat is that filesystem permissions can't be used to
control access to the socket file (since, of course, there's no
file). Anyone knowing the socket name can access to it. The
only method to control access to it is by using
network_namespaces(7). Since in unitd(8) we're using 0666 file
sockets, abstract sockets should be no more insecure than that
(anyone can already read/write to the listener sockets).
- Ask the kernel to implement a simpler way to unlink(2) socket
files when they are not needed anymore. I've suggested that to
the <linux-fsdevel@vger.kernel.org> mailing list, in:
<lore.kernel.org/linux-fsdevel/0bc5f919-bcfd-8fd0-a16b-9f060088158a@gmail.com/T>
In this commit, I decided to go for the easiest/simplest solution,
which is abstract sockets. In fact, we already had partial
support. This commit only fixes some small bug in the existing
code so that abstract Unix sockets work:
- Don't chmod(2) the socket if it's an abstract one.
This fixes the creation of abstract sockets, but doesn't make them
usable, since we produce them with a trailing '\0' in their name.
That will be fixed in the following commit.
This closes#669 issue on GitHub.
