Hi Andrew,
This patch set simplifies the rules for including files, so that
widely-available headers, especially those required by
POSIX.1-2001, are included unconditionally.
Then, headers that may not exist in some systems are included
conditionally to their existence. For that we need to add a new
set of NXT_HAVE_..._H macros created by the new <auto/headers>
script.
This patch set fixes some obscure dependencies between headers,
which when reordering some include files, triggers MacOS to
include <mntent.h>, which doesn't exist in that system. After
this patch set, since the header doesn't exist, it's not included.
This is a prerequisite for even attempting to use iwyu(1).
Cheers,
Alex
Alejandro Colomar (22):
Including <mntent.h> iff it exists.
Including <sys/prctl.h> iff it exists.
Including <linux/openat2.h> iff it exists.
Including <sys/mount.h> unconditionally.
Including <sys/random.h> iff it exists.
Including <sys/sendfile.h> iff it exists.
Including <sys/pollset.h> iff it exists.
Including <sys/devpoll.h> iff it exists.
Including <port.h> iff it exists.
Including <sys/event.h> iff it exists.
Including <sys/eventfd.h> iff it exists.
Including <sys/signalfd.h> iff it exists.
Including <sys/epoll.h> iff it exists.
Including <sys/un.h> iff it exists.
Removed code used when NXT_HAVE_POSIX_SPAWN is false.
Including <sys/mercury.h> iff it exists.
Including <malloc_np.h> iff it exists.
Including <sys/param.h> unconditionally.
Including <sys/uio.h> unconditionally.
Including <sys/syscall.h> and <unistd.h> unconditionally.
Including <linux/memfd.h> iff it exists.
Including <linux/capability.h> iff it exists.
auto/headers | 224 ++++++++++++++++++++++++++++++++++++++++++
auto/unix | 15 ---
configure | 1 +
src/nxt_application.c | 2 +-
src/nxt_capability.c | 8 +-
src/nxt_fs.c | 2 -
src/nxt_isolation.c | 2 +-
src/nxt_port_memory.c | 7 +-
src/nxt_process.c | 50 +---------
src/nxt_process.h | 4 +-
src/nxt_socket_msg.h | 2 +-
src/nxt_unit.c | 2 +-
src/nxt_unix.h | 47 ++++-----
13 files changed, 261 insertions(+), 105 deletions(-)
create mode 100644 auto/headers
--
2.36.1
> But the pointer itself does constitute another separate object. And
that object is the one that is stored in the union (the uint8_t data is
not stored in the union, but in some other allocated memory (probably)).
The pointer to structure's field does not stored in the structure. And reading the pointer to the field is just adding an offset to the object pointer.
Why do you ignore my suggestion to replace "p->base" with "(uint8_t *) p" and try to understand the purpose of the object, 'base' and 'offset'? Instead you digging the specifications to prove the validation software behavior.
--
Max
-----Original Message-----
From: Alejandro Colomar <alx.manpages(a)gmail.com>
To: Max Romanov <max.romanov(a)gmail.com>, Andrew Clayton <andrew(a)digital-domain.net>
Cc: unit(a)nginx.org
Sent: сб, 18 июн. 2022 2:15
Subject: Re: [PATCH 06/11] Sptr: avoided potentially undefined behaviour.
Hi Max
On 6/18/22 01:04, Max Romanov wrote:
> I'm sorry to be a pain,
No problem :)
but the key thing is: ".. read from another
> object..".
In this case, ISO C uses the term object to refer to a broader concept,
that includes any variable.
3.15/1,2:
"
object
region of data storage in the execution environment, the contents of
which can represent values
"
> In the statement you are trying to "fix", the "base" does not
> actually _read_ from the object (structure, to be precise), instead the
> address of the structure itself used. In other words, "p->base" is a
> shortcut for "(uint8_t *) p".
the region of data storage pointed to by `p->base` would be an object
(of type uint8_t, or maybe uint8_t[]).
But the pointer itself does constitute another separate object. And
that object is the one that is stored in the union (the uint8_t data is
not stored in the union, but in some other allocated memory (probably)).
Since we are reading a member of the union (and each member is an
object, by definition, even if it's a pointer), and storing the value
into another member of the union, that's not valid.
As pointed by @supercat in StackOverflow (in the link I provided), the
rationale was probably to allow compilers to do some optimizations, or
for example, copy byte by byte; in archs where the union members are
larger than a register, this will definitely cause a bug; in archs where
the union members fit into a register (as in our case), the code is
likely to work, but the standard makes no guarantees about it, and a
compiler may still do bad stuff. It's better to be safe.
Cheers,
Alex
>> <https://stackoverflow.com/a/72652427/6872717
--
Alejandro Colomar
<http://www.alejandro-colomar.es/>
I'm appreciate your patience and effort to make it clear!
I also admit it is not obvious way to describe the relocatable offset in C.
--
Max
-----Original Message-----
From: Alejandro Colomar <alx.manpages(a)gmail.com>
To: Max Romanov <max.romanov(a)gmail.com>, Andrew Clayton <andrew(a)digital-domain.net>
Cc: unit(a)nginx.org
Sent: сб, 18 июн. 2022 14:00
Subject: Re: [PATCH 06/11] Sptr: avoided potentially undefined behaviour.
Hi Max,
On 6/18/22 11:39, Max Romanov wrote:
> The pointer to structure's field does not stored in the structure. And
> reading the pointer to the field is just adding an offset to the object
> pointer.
Okay, now I got it.
So, p->base is reinterpreting as uint_8[], the contents of some stucture.
p, the pointer to the union, is really a pointer to that structure.
p->base, when used in pointer arithmetics, decays to a pointer to the
first element, which is the same as a pointer to the union, which is the
same as a pointer to the reinterpreted structure.
And p->offset is just an offset to that pointer, so it's the offset of
ptr to the start to the structure (reinterpreted as a uint8_t[]), and
it's stored as the first element of said structure.
Now it makes sense.
So, yes, the patch was wrong, and the linter has a bug (but they'll
probably ignore it because unions in C++ are so crap that it doesn't pay
out fixing it).
>
> Why do you ignore my suggestion to replace "p->base" with "(uint8_t *)
> p"
Sory, I misunderstood it yesterday.
> and try to understand the purpose of the object, 'base' and 'offset'?
> Instead you digging the specifications to prove the validation software
> behavior.
Thanks!
Alex
--
Alejandro Colomar
<http://www.alejandro-colomar.es/>
In src/nxt_port_socket.c::nxt_port_read_msg_process() msg->cancelled is
set to 0 and is not touched again.
However there are several checks for it being == 0 which are _always_
true, so remove them.
I'm assuming that this is functioning as intended and that setting
msg->cancelled to 0 is correct.
msg->cancelled was set to 0 unconditionally in commit e227fc9
("Introducing application and port shared memory queues."), so I guess
the now redundant checks were simply missed for removal.
---
src/nxt_port_socket.c | 27 ++++++++++-----------------
1 file changed, 10 insertions(+), 17 deletions(-)
diff --git a/src/nxt_port_socket.c b/src/nxt_port_socket.c
index 5752d5a..2649e69 100644
--- a/src/nxt_port_socket.c
+++ b/src/nxt_port_socket.c
@@ -1237,7 +1237,7 @@ nxt_port_read_msg_process(nxt_task_t *task, nxt_port_t *port,
} else {
if (nxt_slow_path(msg->port_msg.mf != 0)) {
- if (msg->port_msg.mmap && msg->cancelled == 0) {
+ if (msg->port_msg.mmap) {
nxt_port_mmap_read(task, msg);
b = msg->buf;
}
@@ -1251,25 +1251,18 @@ nxt_port_read_msg_process(nxt_task_t *task, nxt_port_t *port,
fmsg->port_msg.nf = 0;
fmsg->port_msg.mf = 0;
- if (nxt_fast_path(msg->cancelled == 0)) {
- msg->buf = NULL;
- msg->fd[0] = -1;
- msg->fd[1] = -1;
- b = NULL;
+ msg->buf = NULL;
+ msg->fd[0] = -1;
+ msg->fd[1] = -1;
+ b = NULL;
- } else {
- nxt_port_close_fds(msg->fd);
- }
} else {
- if (nxt_fast_path(msg->cancelled == 0)) {
-
- if (msg->port_msg.mmap) {
- nxt_port_mmap_read(task, msg);
- b = msg->buf;
- }
-
- port->handler(task, msg);
+ if (msg->port_msg.mmap) {
+ nxt_port_mmap_read(task, msg);
+ b = msg->buf;
}
+
+ port->handler(task, msg);
}
}
--
2.36.1
I'm sorry to be a pain, but the key thing is: ".. read from another object..". In the statement you are trying to "fix", the "base" does not actually _read_ from the object (structure, to be precise), instead the address of the structure itself used. In other words, "p->base" is a shortcut for "(uint8_t *) p".
--
Max
-----Original Message-----
From: Andrew Clayton <andrew(a)digital-domain.net>
To: Alejandro Colomar <alx.manpages(a)gmail.com>
Cc: unit(a)nginx.org, Max Romanov <max.romanov(a)gmail.com>
Sent: пт, 17 июн. 2022 2:05
Subject: Re: [PATCH 06/11] Sptr: avoided potentially undefined behaviour.
On Fri, 17 Jun 2022 00:17:20 +0200
Alejandro Colomar <alx.manpages(a)gmail.com> wrote:
> Woah, wait a bit. No it doesn't. Today I learnt:
>
> 6.5.16.1/3 (Assignment operators::Simple Assignment::Semantics):
>
> If the value being stored in an object is read from another object that
> overlaps in any way the storage of the first object, then the overlap
> shall be exact and the two objects shall have qualified or unqualified
> versions of a compatible type; otherwise, the behavior is undefined.
>
> <https://www.open-std.org/JTC1/SC22/WG14/www/docs/n2731.pdf>
>
> I thought union would have this issue in C++, but not in C; but it does.
> So this patch is actually correct.
I knew it! ;)
> We have:
>
> $ grepc nxt_unit_sptr_.
> ./src/nxt_unit_sptr.h:18:
> union nxt_unit_sptr_u {
> uint8_t base[1];
> uint32_t offset;
> };
>
>
> ./src/nxt_unit_typedefs.h:18:
> typedef union nxt_unit_sptr_u nxt_unit_sptr_t;
>
>
> Since `uint8_t [1]` (or `uint8_t *`, to which it decays) is incompatible
> with `uint32_t`, it is UB, and storing it in a temporary fixes that.
Thanks for looking into this.
> So, for the patch:
>
> Reviewed-by: Alejandro Colomar <alx.manpages(a)gmail.com>
Well, I'm glad I wasn't completely wrong! I'll maybe tweak the commit
message and quote the spec...
> BTW, updated Stackoverflow: <https://stackoverflow.com/a/72652427/6872717>
And the above paragraph is in at least the C99 spec.
Cheers,
Andrew