f7c9d3a8b3
Andrei reported an issue on arm64 where he was seeing the following
error message when running the tests
2024/01/17 18:32:31.109 [error] 54904#54904 "gidmap" field has an entry with "size": 1, but for unprivileged unit it must be 1.
This error message is guarded by the following if statement
if (nxt_slow_path(m.size > 1)
Turns out size was indeed > 1, in this case it was 289356276058554369,
m.size is defined as a nxt_int_t, which on arm64 is actually 8 bytes,
but was being printed as a signed int (4 bytes) and by chance/undefined
behaviour comes out as 1.
But why is size so big? In this case it should have just been 1 with a
config of
'gidmap': [{'container': 0, 'host': os.getegid(), 'size': 1}],
This is due to nxt_int_t being 64bits on arm64 but using a conf type of
NXT_CONF_MAP_INT which means in nxt_conf_map_object() we would do (using
our m.size variable as an example)
ptr = nxt_pointer_to(data, map[i].offset);
...
ptr->i = num;
Where ptr is a union pointer and is now pointing at our m.size
Next we set m.size to the value of num (which is 1 in this case), via
ptr->i where i is a member of that union of type int.
So here we are setting a 64bit memory location (nxt_int_t on arm64)
through a 32bit (int) union alias, this means we are only setting the
lower half (4) of the bytes.
Whatever happens to be in the upper 4 bytes will remain, giving us our
exceptionally large value.
This is demonstrated by this program
#include <stdio.h>
#include <stdint.h>
int main(void)
{
int64_t num = -1; /* All 1's in two's complement */
union {
int32_t i32;
int64_t i64;
} *ptr;
ptr = (void *)#
ptr->i32 = 1;
printf("num : %lu / %ld\n", num, num);
ptr->i64 = 1;
printf("num : %ld\n", num);
return 0;
}
$ make union-32-64-issue
cc union-32-64-issue.c -o union-32-64-issue
$ ./union-32-64-issue
num : 18446744069414584321 / -4294967295
num : 1
However that is not the only issue, because the members of
nxt_clone_map_entry_t were specified as nxt_int_t's on the likes of
x86_64 this would be a 32bit signed integer. However uid/gids on Linux
at least are defined as unsigned integers, so a nxt_int_t would not be
big enough to hold all potential values.
We could make the nxt_uint_t's but then we're back to the above union
aliasing problem.
We could just set the memory for these variables to 0 and that would
work, however that's really just papering over the problem.
The right thing is to use a large enough sized type to store these
things, hence the previously introduced nxt_cred_t. This is an int64_t
which is plenty large enough.
So we switch the nxt_clone_map_entry_t structure members over to
nxt_cred_t's and use NXT_CONF_MAP_INT64 as the conf type, which then
uses the right sized union member in nxt_conf_map_object() to set these
variables.
Reported-by: Andrei Zeliankou <zelenkov@nginx.com>
Reviewed-by: Zhidao Hong <z.hong@f5.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
1.3 KiB
1.3 KiB