[PATCH 1/3] ipc: avoid dereference of null pointer and quiet the GCC warning about uninitialized variable
The problem is in this statement:
assert((entry == IE_NULL) || IE_BITS_TYPE(entry-ie_bits));
The macro assert() checks for a negation of this expression. Negation of an
OR expression is an AND expression. In order to evaluate an AND expression,
compiler needs to check both conditions. So it first evaluates (entry ==
IE_NULL)
and then IE_BITS_TYPE(entry-ie_bits). If the code path was that where entry is
a
null pointer, the evaluation of the second condition results in a dereference
of a null
pointer. Since this dereference happend, the initialization of notify_port is
never
reached in this code path, and GCC concludes that notify_port is later used
uninitialized.
* ipc/ipc_entry.c (ipc_entry_lookup) (assert): Fix assertion.
* ipc/ipc_kmsg.c (ipc_kmsg_copyin_header) (notify_port): Don't initialize to
zero.
(ipc_kmsg_copyin_header): Break the if statement into two statements to avoid
having a check for null pointer and dereference in one statement.
---
ipc/ipc_entry.c | 2 +-
ipc/ipc_kmsg.c | 9 ++---
2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/ipc/ipc_entry.c b/ipc/ipc_entry.c
index f1d763c..f182baf 100644
--- a/ipc/ipc_entry.c
+++ b/ipc/ipc_entry.c
@@ -123,7 +123,7 @@ ipc_entry_lookup(space, name)
entry = (ipc_entry_t)
ipc_splay_tree_lookup(space-is_tree, name);
- assert((entry == IE_NULL) || IE_BITS_TYPE(entry-ie_bits));
+ assert((entry == IE_NULL) || ((entry != IE_NULL) ?
IE_BITS_TYPE(entry-ie_bits) : TRUE));
return entry;
}
diff --git a/ipc/ipc_kmsg.c b/ipc/ipc_kmsg.c
index 0e43410..270d511 100644
--- a/ipc/ipc_kmsg.c
+++ b/ipc/ipc_kmsg.c
@@ -944,7 +944,7 @@ ipc_kmsg_copyin_header(msg, space, notify)
mach_msg_type_name_t reply_type = MACH_MSGH_BITS_LOCAL(mbits);
ipc_object_t dest_port, reply_port;
ipc_port_t dest_soright, reply_soright;
- ipc_port_t notify_port = 0; /* '=0' to quiet gcc warnings */
+ ipc_port_t notify_port;
if (!MACH_MSG_TYPE_PORT_ANY_SEND(dest_type))
return MACH_SEND_INVALID_HEADER;
@@ -961,8 +961,11 @@ ipc_kmsg_copyin_header(msg, space, notify)
if (notify != MACH_PORT_NULL) {
ipc_entry_t entry;
- if (((entry = ipc_entry_lookup(space, notify)) == IE_NULL) ||
- ((entry-ie_bits MACH_PORT_TYPE_RECEIVE) == 0)) {
+ if ((entry = ipc_entry_lookup(space, notify)) == IE_NULL) {
+ is_write_unlock(space);
+ return MACH_SEND_INVALID_NOTIFY;
+ }
+ if ((entry-ie_bits MACH_PORT_TYPE_RECEIVE) == 0) {
is_write_unlock(space);
return MACH_SEND_INVALID_NOTIFY;
}
--
1.8.1.4
Re: [PATCH 1/3] ipc: avoid dereference of null pointer and quiet the GCC warning about uninitialized variable
On Wed, 18 Dec 2013 09:17:47 +0100 Marin Ramesa m...@hi.t-com.hr wrote: ... Negation of an OR expression is an AND expression. ... Maybe I did't get you correctly, but isn't !(a || b) == !a !b ? And evaluation of the second condition doesn't happen when entry = IE_NULL -- Regards, Anatoly
Re: [PATCH 1/3] ipc: avoid dereference of null pointer and quiet the GCC warning about uninitialized variable
On 18.12.2013 10:20:21, Anatoly A. Kazantsev wrote: On Wed, 18 Dec 2013 09:17:47 +0100 Marin Ramesa m...@hi.t-com.hr wrote: ... Negation of an OR expression is an AND expression. ... Maybe I did't get you correctly, but isn't !(a || b) == !a !b ? Yes. And evaluation of the second condition doesn't happen when entry = IE_NULL Compiler needs to check both !a and !b. In order to evaluate !b it must evaluate b. So when the code path is that when entry is a null pointer, the evaluation of b results in a dereference of a null pointer.
Re: [PATCH 1/3] ipc: avoid dereference of null pointer and quiet the GCC warning about uninitialized variable
On Wed, Dec 18, 2013 at 10:37:03AM +0100, Marin Ramesa wrote: Compiler needs to check both !a and !b. In order to evaluate !b it must evaluate b. So when the code path is that when entry is a null pointer, the evaluation of b results in a dereference of a null pointer. No, that's wrong. The and || operators are guaranteed to be evaluated left-to-right, and yield if the first operand compares equal to 0. And that's exactly why this check against NULL is done first. -- Richard Braun
Re: [PATCH 1/3] ipc: avoid dereference of null pointer and quiet the GCC warning about uninitialized variable
On Wed, Dec 18, 2013 at 10:46:40AM +0100, Richard Braun wrote: On Wed, Dec 18, 2013 at 10:37:03AM +0100, Marin Ramesa wrote: Compiler needs to check both !a and !b. In order to evaluate !b it must evaluate b. So when the code path is that when entry is a null pointer, the evaluation of b results in a dereference of a null pointer. No, that's wrong. The and || operators are guaranteed to be evaluated left-to-right, and yield if the first operand compares equal to 0. And that's exactly why this check against NULL is done first. Compares equal to 0 for , and not equal to 0 for ||, obviously. -- Richard Braun
Re: [PATCH 1/3] ipc: avoid dereference of null pointer and quiet the GCC warning about uninitialized variable
On 18.12.2013 10:46:40, Richard Braun wrote: On Wed, Dec 18, 2013 at 10:37:03AM +0100, Marin Ramesa wrote: Compiler needs to check both !a and !b. In order to evaluate !b it must evaluate b. So when the code path is that when entry is a null pointer, the evaluation of b results in a dereference of a null pointer. No, that's wrong. The and || operators are guaranteed to be evaluated left-to-right, and yield if the first operand compares equal to 0. And that's exactly why this check against NULL is done first. In the expression (!a !b), if !a equals 0, the compiler must check !b == 0 in order to return TRUE. If !a equals 0, that means the entry is a null pointer, and evaluation of !b is a dereference of a null pointer.
Re: [PATCH 1/3] ipc: avoid dereference of null pointer and quiet the GCC warning about uninitialized variable
On 18.12.2013 10:55:47, Marin Ramesa wrote: in order to return TRUE Sorry, I meant to say in order to return FALSE.
Re: [PATCH 1/3] ipc: avoid dereference of null pointer and quiet the GCC warning about uninitialized variable
On Wed, Dec 18, 2013 at 10:55:47AM +0100, Marin Ramesa wrote: On 18.12.2013 10:46:40, Richard Braun wrote: No, that's wrong. The and || operators are guaranteed to be evaluated left-to-right, and yield if the first operand compares equal to 0. And that's exactly why this check against NULL is done first. In the expression (!a !b), if !a equals 0, the compiler must check !b == 0 in order to return TRUE. If !a equals 0, that means the entry is a null pointer, and evaluation of !b is a dereference of a null pointer. The expression is ((a == NULL) || a-something), and I agree it is equivalent to !((a != NULL) !a-something). And again, both the and || operators are guaranteed to be evaluated left-to-right and *yield* without evaluating the second operand if the first compares or not to 0, depending on the operator. So, let's take the seconds form since that's what you've used, without the negation for simplicity : ((a != NULL) !a-something) If a isn't NULL, then it returns !a-something. If a is NULL, then (a != NULL) compares equal to 0, and returns 0 before evaluating !a-something. So no, there can't be a null pointer dereference here. And this really is basic C, so please double check your changes. -- Richard Braun
Re: [PATCH 1/3] ipc: avoid dereference of null pointer and quiet the GCC warning about uninitialized variable
On 18.12.2013 11:11:10, Richard Braun wrote: The expression is ((a == NULL) || a-something), and I agree it is equivalent to !((a != NULL) !a-something). And again, both the and || operators are guaranteed to be evaluated left-to-right and *yield* without evaluating the second operand if the first compares or not to 0, depending on the operator. So, let's take the seconds form since that's what you've used, without the negation for simplicity : ((a != NULL) !a-something) If a isn't NULL, then it returns !a-something. If a is NULL, then (a != NULL) compares equal to 0, and returns 0 before evaluating !a-something. So no, there can't be a null pointer dereference here. And this really is basic C, so please double check your changes. Yes, vou're right. I got confused. But then something else is happening here. When I write the assertion this way: assert((entry == IE_NULL) || ((entry != IE_NULL) ? IE_BITS_TYPE (entry-ie_bits) : TRUE)); GCC stops complaing about uninitialized variable.
Re: [PATCH 1/3] ipc: avoid dereference of null pointer and quiet the GCC warning about uninitialized variable
Marin Ramesa m...@hi.t-com.hr writes:
On 18.12.2013 10:46:40, Richard Braun wrote:
[...]
No, that's wrong. The and || operators are guaranteed to be
evaluated left-to-right, and yield if the first operand compares
equal to 0. And that's exactly why this check against NULL is done
first.
In the expression (!a !b), if !a equals 0, the compiler must check
!b == 0 in order to return [FALSE]. If !a equals 0, that means the
entry is a null pointer, and evaluation of !b is a dereference of a
null pointer.
Well, when explaining these operators to the students, I’d
rather call them “conditionals” (along with A ? B : C and ‘if’.)
Specifically:
• the A B operator evaluates the A expression; if the result
is zero, zero is returned; if the result is non-zero, the B
expression is evaluated, and its result is returned;
• the A || B operator evaluates the A expression; if the result
is non-zero, it is returned; if the result is zero, the B
expression is evaluated, and its result is returned.
Essentially, A B is equivalent to (A ? B : 0), while A || B
is equivalent to (A ? A : B), except that in latter case, A is
evaluated only once. (Thus A++ || B doesn’t result in A being
incremented twice, as would be in the case of A++ ? A++ : B.)
One may wish to check the following simplictic C code.
#include stdio.h
int
main ()
{
int a = 0, b = 42;
int c
= a puts (a is true);
int d
= a || puts (a is false);
int e
= b puts (b is true);
int f
= b || puts (b is false);
/* . */
return 0;
}
Note that only some of the puts () calls are in fact evaluated.
--
FSF associate member #7257
Re: [PATCH 1/3] ipc: avoid dereference of null pointer and quiet the GCC warning about uninitialized variable
On Wed, Dec 18, 2013 at 11:25:36AM +0100, Marin Ramesa wrote: Yes, vou're right. I got confused. But then something else is happening here. When I write the assertion this way: assert((entry == IE_NULL) || ((entry != IE_NULL) ? IE_BITS_TYPE (entry-ie_bits) : TRUE)); GCC stops complaing about uninitialized variable. I don't get this warning, can you tell us how you configure GNU Mach ? -- Richard Braun
Re: [PATCH 1/3] ipc: avoid dereference of null pointer and quiet the GCC warning about uninitialized variable
On 18.12.2013 11:34:03, Richard Braun wrote: On Wed, Dec 18, 2013 at 11:25:36AM +0100, Marin Ramesa wrote: Yes, vou're right. I got confused. But then something else is happening here. When I write the assertion this way: assert((entry == IE_NULL) || ((entry != IE_NULL) ? IE_BITS_TYPE (entry-ie_bits) : TRUE)); GCC stops complaing about uninitialized variable. I don't get this warning, can you tell us how you configure GNU Mach ? --enable-kdb --prefix= But the warning is turned off by: ipc_port_t notify_port = 0; in ipc/ipc_kmsg.c. I don't think that's a good solution. I think something else is happening here.
Re: [PATCH 1/3] ipc: avoid dereference of null pointer and quiet the GCC warning about uninitialized variable
On Wed, Dec 18, 2013 at 11:40:09AM +0100, Marin Ramesa wrote: On 18.12.2013 11:34:03, Richard Braun wrote: I don't get this warning, can you tell us how you configure GNU Mach ? --enable-kdb --prefix= But the warning is turned off by: ipc_port_t notify_port = 0; in ipc/ipc_kmsg.c. I don't think that's a good solution. I think something else is happening here. How can a local variable in ipc_kmsg.c have any effect on another local variable in ipc_entry.c ?? -- Richard Braun
Re: [PATCH 1/3] ipc: avoid dereference of null pointer and quiet the GCC warning about uninitialized variable
On 18.12.2013 12:00:49, Richard Braun wrote: On Wed, Dec 18, 2013 at 11:40:09AM +0100, Marin Ramesa wrote: On 18.12.2013 11:34:03, Richard Braun wrote: I don't get this warning, can you tell us how you configure GNU Mach ? --enable-kdb --prefix= But the warning is turned off by: ipc_port_t notify_port = 0; in ipc/ipc_kmsg.c. I don't think that's a good solution. I think something else is happening here. How can a local variable in ipc_kmsg.c have any effect on another local variable in ipc_entry.c ?? Yes, my change doesn't clear the warning. I forgot to do 'make clean' and rebuild. The patch is completely wrong.
