Some non-Linux systems implement pivot_root(2), even if they
don't document that. An example is MacOS:
$ grepc pivot_root / 2>/dev/null
.../sys/sysproto.h:3012:
int pivot_root(struct proc *, struct pivot_root_args *, int *);
Since the prototype of the syscall differs from that of Linux, we
can't use that syscall. Let's make sure the test only detects
pivot_root(2) under Linux. Also, rename the feature macro to make
clear that it's only about Linux's pivot_root(2).
This closes#737 issue on GitHub.
With NXT_HAVE_PIVOT_ROOT, we had issues in MacOS. Headers should
normally be included unconditionally, except of course if they
don't exist.
This fixes part of the #737 issue on GitHub.
Having the basename of the script pathname was incorrect. While
we don't have something more accurate, the best thing to do is to
have it empty (which should be the right thing most of the time).
This closes#715 issue on GitHub.
The bug was introduced in git commit
0032543fa6
'Ruby: added the Rack environment parameter "SCRIPT_NAME".'.
The #endif was misplaced by accident during a refactor:
<https://github.com/nginx/unit/commit/029942f4eb7196c2cff0d0e26bc6ff274138f7d8>.
clang(1)'s -Wunreachable-code-break (implied by -Weverything) catches
that, but it is only produced for code compiled without support
for Unix sockets, which is probably the reason it was undetected:
no-one seems to be compiling Unit without Unix sockets support (at
least with clang(1)).
Some OSes, as Linux, provide FIONBIO in <sys/ioctl.h>. Others,
such as the BSDs and Illumos, provide it in <sys/filio.h>, but
they all include that header from <sys/ioctl.h>, so for this test,
we can simplify and just include <sys/ioctl.h>.
posix_spawn(3POSIX) was introduced by POSIX.1d
(IEEE Std 1003.1d-1999), and was later consolidated in
POSIX.1-2001, requiring it in all POSIX-compliant systems.
It's safe to assume it's always available, more than 20 years
after its standardization.
Link: <https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/spawn.h.html>
That is already handled in auto/options. It is better suited
there, since it's an option, and not a module, and also because it
makes use of variables declared there.
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.
Casts are usually very dangerous, disabling most compiler warnings
and basically removing type safety. This change adds 'const' to a
pointer where we don't need to write, improving type safety, and
that also allows removing some casts.
nxt_str_null() setted the loc.start pointer to NULL, which was
being passed to memcpy(3) through nxt_debug(). That caused
Undefined Behavior, so we now pass an empty string.
An empty string in Location was being handled specially by not sending a
Location header. This may occur after variable resolution, so we need to
consider this scenario.
The obsolete RFC 2616 defined the Location header as consisting of an absolute
URI <https://www.rfc-editor.org/rfc/rfc2616#section-14.30>, which cannot be an
empty string. However, the current RFC 7231 allows the Location to be a
relative URI <https://www.rfc-editor.org/rfc/rfc7231#section-7.1.2>, and a
relative URI may be an empty string <https://stackoverflow.com/a/43338457>.
Due to these considerations, this patch allows sending an empty Location header
without handling this case specially. This behavior will probably be more
straightforward to users, too. It also simplifies the code, which is now more
readable, fast, and conformant to the current RFC. We're skipping an
allocation at request time in a common case such as "action": {"return": 404}
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'.
We had a mix of styles for declaring function-like macros:
Style A:
#define \
foo() \
do { \
... \
} while (0)
Style B:
#define foo() \
do { \
... \
} while (0)
We had a similar number of occurences of each style:
$ grep -rnI '^\w*(.*\\' | wc -l
244
$ grep -rn 'define.*(.*)' | wc -l
239
(Those regexes aren't perfect, but a very decent approximation.)
Real examples:
$ find src -type f | xargs sed -n '/^nxt_double_is_zero/,/^$/p'
nxt_double_is_zero(f) \
(fabs(f) <= FLT_EPSILON)
$ find src -type f | xargs sed -n '/define nxt_http_field_set/,/^$/p'
#define nxt_http_field_set(_field, _name, _value) \
do { \
(_field)->name_length = nxt_length(_name); \
(_field)->value_length = nxt_length(_value); \
(_field)->name = (u_char *) _name; \
(_field)->value = (u_char *) _value; \
} while (0)
I'd like to standardize on a single style for them, and IMO,
having the identifier in the same line as #define is a better
option for the following reasons:
- Programmers are used to `#define foo() ...` (readability).
- One less line of code.
- The program for finding them is really simple (see below).
function grep_ngx_func()
{
if (($# != 1)); then
>&2 echo "Usage: ${FUNCNAME[0]} <func>";
return 1;
fi;
find src -type f \
| grep '\.[ch]$' \
| xargs grep -l "$1" \
| sort \
| xargs pcregrep -Mn "(?s)^\$[\w\s*]+?^$1\(.*?^}";
find src -type f \
| grep '\.[ch]$' \
| xargs grep -l "$1" \
| sort \
| xargs pcregrep -Mn "(?s)define $1\(.*?^$" \
| sed -E '1s/^[^:]+:[0-9]+:/&\n\n/';
}
$ grep_ngx_func
Usage: grep_ngx_func <func>
$ grep_ngx_func nxt_http_field_set
src/nxt_http.h:98:
#define nxt_http_field_set(_field, _name, _value) \
do { \
(_field)->name_length = nxt_length(_name); \
(_field)->value_length = nxt_length(_value); \
(_field)->name = (u_char *) _name; \
(_field)->value = (u_char *) _value; \
} while (0)
$ grep_ngx_func nxt_sprintf
src/nxt_sprintf.c:56:
u_char * nxt_cdecl
nxt_sprintf(u_char *buf, u_char *end, const char *fmt, ...)
{
u_char *p;
va_list args;
va_start(args, fmt);
p = nxt_vsprintf(buf, end, fmt, args);
va_end(args);
return p;
}
................
Scripted change:
................
$ find src -type f \
| grep '\.[ch]$' \
| xargs sed -i '/define *\\$/{N;s/ *\\\n/ /;s/ //}'
............
Description:
............
Before this commit, the encoded URI could be calculated at
configuration time. Now, since variables can only be resolved at
request time, we have different situations:
- "location" contains no variables:
In this case, we still encode the URI in the conf structure, at
configuration time, and then we just copy the resulting string
to the ctx structure at request time.
- "location" contains variables:
In this case, we compile the var string at configure time, then
when we resolve it at request time, and then we encode the
string.
In both cases, as was being done before, if the string is empty,
either before or after resolving variables, we skip the encoding.
...........
Usefulness:
...........
An example of why this feature may be useful is redirecting HTTP
to HTTPS with something like:
"action": {
"return": 301,
"location": "https://${host}${uri}"
}
.....
Bugs:
.....
This feature conflicts with the relevant RFCs in the following:
'$' is used for Unit variables, but '$' is a reserved character in
a URI, to be used as a sub-delimiter. However, it's almost never
used as that, and in fact, other parts of Unit already conflict
with '$' being a reserved character for use as a sub-delimiter, so
this is at least consistent in that sense. VBart suggested an
easy workaround if we ever need it: adding a variable '$sign'
which resolves to a literal '$'.
......
Notes:
......
An empty string is handled as if "location" wasn't specified at
all, so no Location header is sent.
This is incorrect, and the code is slightly misleading.
The Location header consists of a URI-reference[1], which might be
a relative one, which itself might consist of an empty string[2].
[1]: <https://www.rfc-editor.org/rfc/rfc7231#section-7.1.2>
[2]: <https://stackoverflow.com/a/43338457>
Now that we have variables, it's more likely that an empty
Location header will be requested, and we should handle it
correctly.
I think in a future commit we should modify the code to allow
differentiating between an unset "location" and an empty one,
which should be treated as any other "location" string.
.................
Testing (manual):
.................
{
"listeners": {
"*:80": {
"pass": "routes/str"
},
"*:81": {
"pass": "routes/empty"
},
"*:82": {
"pass": "routes/var"
},
"*:83": {
"pass": "routes/enc-str"
},
"*:84": {
"pass": "routes/enc-var"
}
},
"routes": {
"str": [
{
"action": {
"return": 301,
"location": "foo"
}
}
],
"empty": [
{
"action": {
"return": 301,
"location": ""
}
}
],
"var": [
{
"action": {
"return": 301,
"location": "$host"
}
}
],
"enc-str": [
{
"action": {
"return": 301,
"location": "f%23o#o"
}
}
],
"enc-var": [
{
"action": {
"return": 301,
"location": "f%23o${host}#o"
}
}
]
}
}
$ curl --dump-header - localhost:80
HTTP/1.1 301 Moved Permanently
Location: foo
Server: Unit/1.27.0
Date: Thu, 07 Apr 2022 23:30:06 GMT
Content-Length: 0
$ curl --dump-header - localhost:81
HTTP/1.1 301 Moved Permanently
Server: Unit/1.27.0
Date: Thu, 07 Apr 2022 23:30:08 GMT
Content-Length: 0
$ curl --dump-header - localhost:82
HTTP/1.1 301 Moved Permanently
Location: localhost
Server: Unit/1.27.0
Date: Thu, 07 Apr 2022 23:30:15 GMT
Content-Length: 0
$ curl --dump-header - -H "Host: bar" localhost:82
HTTP/1.1 301 Moved Permanently
Location: bar
Server: Unit/1.27.0
Date: Thu, 07 Apr 2022 23:30:23 GMT
Content-Length: 0
$ curl --dump-header - -H "Host: " localhost:82
HTTP/1.1 301 Moved Permanently
Server: Unit/1.27.0
Date: Thu, 07 Apr 2022 23:30:29 GMT
Content-Length: 0
$ curl --dump-header - localhost:83
HTTP/1.1 301 Moved Permanently
Location: f%23o#o
Server: Unit/1.27.0
Date: Sat, 09 Apr 2022 11:22:23 GMT
Content-Length: 0
$ curl --dump-header - -H "Host: " localhost:84
HTTP/1.1 301 Moved Permanently
Location: f%23o#o
Server: Unit/1.27.0
Date: Sat, 09 Apr 2022 11:22:44 GMT
Content-Length: 0
$ curl --dump-header - -H "Host: alx" localhost:84
HTTP/1.1 301 Moved Permanently
Location: f%23oalx#o
Server: Unit/1.27.0
Date: Sat, 09 Apr 2022 11:22:52 GMT
Content-Length: 0
$ curl --dump-header - -H "Host: a#l%23x" localhost:84
HTTP/1.1 301 Moved Permanently
Location: f%2523oa#l%2523x%23o
Server: Unit/1.27.0
Date: Sat, 09 Apr 2022 11:23:09 GMT
Content-Length: 0
$ curl --dump-header - -H "Host: b##ar" localhost:82
HTTP/1.1 301 Moved Permanently
Location: b#%23ar
Server: Unit/1.27.0
Date: Sat, 09 Apr 2022 11:25:01 GMT
Content-Length: 0
Add -fdeclspec to NXT_RUBY_CFLAGS for Clang, if it's available.
Clang incorrectly reports 1 for __has_declspec_attribute(x) in
some cases, such as MacOS or Cygwin. That causes ruby code to
break. ruby added -fdeclspec to their CFLAGS in 2019 to
workaround this bug, since it enables __declspec() and therefore,
the compiler behavior matches what it reports.
Since we don't know what are all the architectures that trigger
the clang bug, let's add the flag for all of them (especially
since it should be harmless).
Add this workaround only at the time of configuring the ruby
module. This way we don't clutter the global NXT_CFLAGS with an
unnecessary flag.
Link: unit bug <https://github.com/nginx/unit/issues/653>
Link: ruby bug <https://bugs.ruby-lang.org/issues/18616>
Link: LLVM bug <https://github.com/llvm/llvm-project/issues/49958>
Commit: LLVM: Add -fdeclspec <d170c4b57a91adc74ca89c6d4af616a00323b12c>
Commit: ruby: Use -fdeclspec <0958e19ffb047781fe1506760c7cbd8d7fe74e57>
When testing some configurations of compilers and OSes, I noticed
that clang(1) 13 on Debian caused a function to be compiled but
unused, and the compiler triggered a compile error.
To avoid that error, use __attribute__((__unused__)). Let's call
our wrapper NXT_MAYBE_UNUSED, since it describes itself more
precisely than the GCC attribute name. It's also the name that
C2x (likely C23) has given to the standard attribute, which is
[[maybe_unused]], so it's also likely to be more readable because
of that name being in ISO C.
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.
Similar to how C pointers to variables can always be considered as
pointers to the first element of an array of size 1 (see the
following code for an example of how they are equivalent),
treating non-NXT_CONF_VALUE_ARRAY as if they were
NXT_CONF_VALUE_ARRAYs of size 1 allows for simpler and more
generic code.
void foo(ptrdiff_t sz, int arr[sz])
{
for (ptrdiff_t i = 0; i < sz; i++)
arr[i] = 0;
}
void bar(void)
{
int x;
int y[1];
foo(1, &x);
foo(1, y);
}
nxt_conf_array_elements_count_or_1():
Similar to nxt_conf_array_elements_count().
Return a size of 1 when input is non-array, instead of
causing undefined behavior. That value (1) makes sense
because it will be used as the limiter of a loop that
loops over the array and calls
nxt_conf_get_array_element_or_itself(), which will return
a correct element for such loops.
nxt_conf_get_array_element_or_itself():
Similar to nxt_conf_get_array_element().
Return the input pointer unmodified (i.e., a pointer to
the unique element of a hypothetical array), instead of
returning NULL, which wasn't very useful.
nxt_conf_array_qsort():
Since it's a no-op for non-arrays, this API can be reused.
Alejandro Colomar