Age | Commit message (Collapse) | Author | Files | Lines |
|
|
|
Refactor.
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
Cc: Zhidao Hong <z.hong@f5.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
|
|
On GH, @tonychuuy reported an issue when using Units 'share' action they
would get the following error in the unit log
2024/01/15 17:53:41 [error] 49#52 *103 file "/var/www/html/public/vendor/telescope/app.css" has changed while sending response to a client
This would happen when trying to serve files over a certain size and the
requested file would not be sent.
This is due to a somewhat bogus check in
nxt_http_static_buf_completion()
I say bogus because it's not clear what the check is trying to
accomplish and the error message is not entirely accurate either.
The check in question goes like
n = pread(file->fd, buf, size, offset);
return n;
...
if (n != size) {
if (n >= 0) {
/* log file changed error and finish */
/* >> Problem is here << */
}
/* log general error and finish */
}
If the number of bytes read is not what we asked for and is > -1 (i.e
not an error) then it says the file has changed, but really it only
checks if the file has _shrunk_ (we can't get back _more_ bytes than we
asked for) since it was stat'd.
This is what happens
recvfrom(22, "GET /tfile HTTP/1.1\r\nHost: local"..., 2048, 0, NULL, NULL) = 82
openat(AT_FDCWD, "/mnt/9p/tfile", O_RDONLY|O_NONBLOCK) = 23
newfstatat(23, "", {st_mode=S_IFREG|0644, st_size=149922, ...}, AT_EMPTY_PATH) = 0
We get a request from a client, open the requested file and stat(2) it to
get the file size.
We would then go into a pread/writev loop reading the file data and
sending it to the client until it's all been sent.
However what was happening in this case was this (showing a dummy file
of 149922 bytes)
pread64(23, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 131072, 0) = 61440
write(2, "2024/01/17 15:30:50 [error] 1849"..., 109) = 109
We wanted to read 131072 bytes but only read 61440 bytes, the above
check triggered and the file transfer was aborted and the above error
message logged.
Normally for a regular file you will only get less bytes than asked for
if the read call is interrupted by a signal or you're near the end of
file.
There is however at least another situation where this may happen, if
the file in question is being served from a network filesystem.
It turns out that was indeed the case here, the files where being served
over the 9P filesystem protocol. Unit was running in a docker container
in an Ubuntu VM under Windows/WSL2 and the files where being passed
through to the VM from Windows over 9P.
Whatever the intention of this check, it is clearly causing issues in
real world scenarios.
If it was really desired to check if the had changed since it was
opened/stat'd then it would require a different methodology and be a
patch for another day. But as it stands this current check does more
harm than good, so lets just remove it.
With it removed we now get for the above test file
recvfrom(22, "GET /tfile HTTP/1.1\r\nHost: local"..., 2048, 0, NULL, NULL) = 82
openat(AT_FDCWD, "/mnt/9p/tfile", O_RDONLY|O_NONBLOCK) = 23
newfstatat(23, "", {st_mode=S_IFREG|0644, st_size=149922, ...}, AT_EMPTY_PATH) = 0
mmap(NULL, 135168, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f367817b000
pread64(23, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 131072, 0) = 61440
pread64(23, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 18850, 61440) = 18850
writev(22, [{iov_base="HTTP/1.1 200 OK\r\nLast-Modified: "..., iov_len=171}, {iov_base="\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., iov_len=61440}, {iov_base="\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., iov_len=18850}], 3) = 80461
pread64(23, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 69632, 80290) = 61440
pread64(23, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 8192, 141730) = 8192
close(23) = 0
writev(22, [{iov_base="\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., iov_len=61440}, {iov_base="\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., iov_len=8192}], 2) = 69632
So we can see we do two pread(2)s's and a writev(2), then another two
pread(2)s and another writev(2) and all the file data has been read and
sent to the client.
Reported-by: tonychuuy <https://github.com/tonychuuy>
Link: <https://en.wikipedia.org/wiki/9P_(protocol)>
Fixes: 08a8d1510 ("Basic support for serving static files.")
Closes: https://github.com/nginx/unit/issues/1064
Reviewed-by: Zhidao Hong <z.hong@f5.com>
Reviewed-by: Andrei Zeliankou <zelenkov@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
|
No functional changes.
|
|
- Configuration: added "/config/settings/http/log_route".
Type: bool
Default: false
This adds configurability to the error log. It allows enabling and
disabling logs related to how the router performs selection of the
routes.
- HTTP: logging request line.
Log level: [notice]
The request line is essential to understand which logs correspond to
which request when reading the logs.
- HTTP: logging route that's been discarded.
Log level: [info]
- HTTP: logging route whose action is selected.
Log level: [notice]
- HTTP: logging when "fallback" action is taken.
Log level: [notice]
Closes: <https://github.com/nginx/unit/issues/758>
Link: <https://github.com/nginx/unit/pull/824>
Link: <https://github.com/nginx/unit/pull/839>
Suggested-by: Timo Stark <t.stark@nginx.com>
Suggested-by: Mark L Wood-Patrick <mwoodpatrick@gmail.com>
Suggested-by: Liam Crilly <liam@nginx.com>
Tested-by: Liam Crilly <liam@nginx.com>
Acked-by: Artem Konev <a.konev@f5.com>
Cc: Andrew Clayton <a.clayton@nginx.com>
Cc: Andrei Zeliankou <zelenkov@nginx.com>
Reviewed-by: Zhidao Hong <z.hong@f5.com>
Signed-off-by: Alejandro Colomar <alx@nginx.com>
|
|
|
|
It's for the introduction of njs support.
For each option that supports native variable and JS template literals introduced next,
it's unified as template string.
No functional changes.
|
|
|
|
This commit adds the variables $arg_NAME, $header_NAME, and $cookie_NAME.
|
|
As was pointed out by the cppcheck[0] static code analysis utility we
can mark numerous function parameters as 'const'. This acts as a hint to
the compiler about our intentions and the compiler will tell us when we
deviate from them.
[0]: https://cppcheck.sourceforge.io/
|
|
The code for finding the extension made a few assumptions that are
no longer true. It didn't account for pathnames that didn't
contain '/', including the empty string, or the NULL string. That
code was used with "share", which always had a '/', but now it's
also used with "index", which should not have a '/' in it.
This fix works by limiting the search to the beginning of the
string, so that if no '/' is found in it, it doesn't continue
searching before the beginning of the string.
This also happens to work for NULL. It is technically Undefined
Behavior, as we rely on `NULL + 0 == NULL` and `NULL - NULL == 0`.
But that is the only sane behavior for an implementation, and all
existing POSIX implementations will Just Work for this code.
Relying on this UB is useful, because we don't need to add an
explicit check for NULL, and therefore we have faster code.
Although the current code can't have a NULL, I expect that when we
add support for variables in the index, it will be NULL in some
cases.
Link: <https://stackoverflow.com/q/67291052/6872717>
The same code seems to be defined behavior in C++, which normally
will share implementation in the compiler for these cases, and
therefore it is really unlikely to be in trouble.
Link: <https://stackoverflow.com/q/59409034/6872717>
|
|
This supports a new option "index" that configures a custom index
file name to be served when a directory is requested. This
initial support only allows a single fixed string. An example:
{
"share": "/www/data/static/$uri",
"index": "lookatthis.htm"
}
When <example.com/foo/bar/> is requested,
</www/data/static/foo/bar/lookatthis.html> is served.
Default is "index.html".
===
nxt_conf_validator.c:
Accept "index" as a member of "share", and make sure it's a string.
===
I tried this feature in my own computer, where I tried the
following:
- Setting "index" to "lookatthis.htm", and check that the correct
file is being served (check both a different name and a
different extension).
- Not setting "index", and check that <index.html> is being
served.
- Settind "index" to an array of strings, and check that the
configuration fails:
{
"error": "Invalid configuration.",
"detail": "The \"index\" value must be a string, but not an array."
}
|
|
Before this patch, if "index" was a file, but not a regular file
nor a directory, so it may have been for example a FIFO, Unit
returned 404. But if "index" was a directory, Unit returned 301.
For consistency, this patch makes Unit return 404 for every
non-regular file, including directories.
|
|
Having a configurable index filename will require adding an index
field to this structure. The most natural name for that field is
'index', so the current index field should be renamed to allow for
that. A sensible name is 'share_idx', since it's the index of the
shares array in 'nxt_http_static_conf_t'.
Instead of 'share_index' I opted for the shorter 'share_idx'.
Also, when 'index' allows an array of filenames in a following
commit, another similar variable 'index_idx' should be created,
and having a different prefix and suffix seems more readable than
for example 'index_index'.
|
|
The previous commit added more generic APIs for handling
NXT_CONF_VALUE_ARRAY and non-NXT_CONF_VALUE_ARRAY together.
Modify calling code to remove special cases for arrays and
non-arrays, taking special care that the path for non arrays is
logically equivalent to the previous special cased code.
Use the now-generic array code only.
|
|
It's not needed after 69d823e5710a.
Found by Clang Static Analyzer.
|
|
|
|
|
|
This commit supports variable in the "share" option, the finding path to
file serve is the value from "share". An example:
{
"share": "/www/data/static$uri"
}
|
|
|
|
No functional changes.
|
|
|
|
|
|
|
|
|
|
|
|
Support for chrooting, rejecting symlinks, and rejecting crossing mounting
points on a per-request basis during static file serving.
|
|
This is a prerequisite for further introduction of openat2() features.
No functional changes.
|
|
When a static file larger than NXT_HTTP_STATIC_BUF_SIZE (128K) is served, two
buffers are allocated and chained; each retains the whole request memory pool.
Starting from 41331471eee7, the completion handler was called once for a linked
buffer chain, but the second buffer got lost.
This patch improves the completion handler's treatment of static buffers to
handle all linked buffers.
|
|
AVIF is a modern image format based on the AV1 video codec. It generally has
better compression than other widely used formats (WebP, JPEG, PNG, and GIF)
and is designed to supersede them. Support was already added to the latest
version of Chrome.
APNG extends PNG to permit animated images that work similarly to animated GIF.
It's supported by most modern browsers.
Also removed duplicated ".svg" entry.
|
|
According to the C standard, pointer arguments passed to memcpy() calls shall
still have valid values. NULL is considered as invalid.
Found with GCC Static Analyzer.
|
|
It allows proceeding to another action if a file isn't available.
An example:
{
"share": "/data/www/",
"fallback": {
"pass": "applications/php"
}
}
In the example above, an attempt is made first to serve a request with
a file from the "/data/www/" directory. If there's no such file, the
request is passed to the "php" application.
Fallback actions may be nested:
{
"share": "/data/www/",
"fallback": {
"share": "/data/cache/",
"fallback": {
"proxy": "http://127.0.0.1:9000"
}
}
}
|
|
|
|
|
|
It's now similar to how attempts to access other non-regular files are handled.
|
|
Found by Coverity (CID 349483).
|
|
|