Re: [systemd-devel] [PATCH] bus: fix null pointer dereference

[Lennart Poettering]

On Sun, 09.11.14 15:41, Ronny Chevalier (chevalier.ro...@gmail.com) wrote:

> CID#1237620
> ---
>  src/libsystemd/sd-bus/bus-message.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/src/libsystemd/sd-bus/bus-message.c 
> b/src/libsystemd/sd-bus/bus-message.c
> index be36d9f..edadacf 100644
> --- a/src/libsystemd/sd-bus/bus-message.c
> +++ b/src/libsystemd/sd-bus/bus-message.c
> @@ -2048,6 +2048,7 @@ static int bus_message_close_variant(sd_bus_message *m, 
> struct bus_container *c)
>  
>  assert(m);
>  assert(c);
> +assert(c->signature);
>  
>  if (!BUS_MESSAGE_IS_GVARIANT(m))
>  return 0;
> @@ -2174,6 +2175,8 @@ _public_ int 
> sd_bus_message_close_container(sd_bus_message *m) {
>  if (c->enclosing != SD_BUS_TYPE_ARRAY)
>  if (c->signature && c->signature[c->index] != 0)
>  return -EINVAL;
> +if (!c->signature && c->enclosing == SD_BUS_TYPE_VARIANT)
> +return -EINVAL;
>  
>  m->n_containers--;

The letter check is really unnecessary, we don't need to fail in this
case. 

Note that when a container is being put together the signature is
known and initialized at the time the container is opened. Hence when
a container was successfully opened, we know that we can also close it
again with an initialized signature.

The only reason why we allow the signature to be NULL in most cases,
is that when no container is open, and we hence put together the
top-level message object, then we don't know the signature yet, and it
grows as elements are added.

It's a bit of a weird assymetry in that regard: containers know the
signature in advance, but the message itself grows the signature as we
go... 

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] bus: fix null pointer dereference

[David Herrmann]

Hi

On Sun, Nov 9, 2014 at 3:41 PM, Ronny Chevalier
 wrote:
> CID#1237620
> ---
>  src/libsystemd/sd-bus/bus-message.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/src/libsystemd/sd-bus/bus-message.c 
> b/src/libsystemd/sd-bus/bus-message.c
> index be36d9f..edadacf 100644
> --- a/src/libsystemd/sd-bus/bus-message.c
> +++ b/src/libsystemd/sd-bus/bus-message.c
> @@ -2048,6 +2048,7 @@ static int bus_message_close_variant(sd_bus_message *m, 
> struct bus_container *c)
>
>  assert(m);
>  assert(c);
> +assert(c->signature);
>
>  if (!BUS_MESSAGE_IS_GVARIANT(m))
>  return 0;
> @@ -2174,6 +2175,8 @@ _public_ int 
> sd_bus_message_close_container(sd_bus_message *m) {
>  if (c->enclosing != SD_BUS_TYPE_ARRAY)
>  if (c->signature && c->signature[c->index] != 0)
>  return -EINVAL;
> +if (!c->signature && c->enclosing == SD_BUS_TYPE_VARIANT)
> +return -EINVAL;

I think we expect "c->signature" to always be non-NULL. See
sd_bus_message_enter_container() and sd_bus_message_open_container().
They call strdup() on the signature unconditionally and I cannot see
another place that allocates bus_container objects.

I'll leave this to Lennart, as he wrote that code. If c->signature is
always non-NULL, we should probably remove the "if (c->signature &&"
part in the line above.

Thanks
David

>  m->n_containers--;
>
> --
> 2.1.3
>
> ___
> systemd-devel mailing list
> systemd-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [PATCH] bus: fix null pointer dereference

[Ronny Chevalier]

CID#1237620
---
 src/libsystemd/sd-bus/bus-message.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/libsystemd/sd-bus/bus-message.c 
b/src/libsystemd/sd-bus/bus-message.c
index be36d9f..edadacf 100644
--- a/src/libsystemd/sd-bus/bus-message.c
+++ b/src/libsystemd/sd-bus/bus-message.c
@@ -2048,6 +2048,7 @@ static int bus_message_close_variant(sd_bus_message *m, 
struct bus_container *c)
 
 assert(m);
 assert(c);
+assert(c->signature);
 
 if (!BUS_MESSAGE_IS_GVARIANT(m))
 return 0;
@@ -2174,6 +2175,8 @@ _public_ int 
sd_bus_message_close_container(sd_bus_message *m) {
 if (c->enclosing != SD_BUS_TYPE_ARRAY)
 if (c->signature && c->signature[c->index] != 0)
 return -EINVAL;
+if (!c->signature && c->enclosing == SD_BUS_TYPE_VARIANT)
+return -EINVAL;
 
 m->n_containers--;
 
-- 
2.1.3

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel