[systemd-devel] [RFC][PATCH 1/2] resolve: resolved-manager: Avoid null dereference

[David Herrmann]

Hi

On Sat, Sep 13, 2014 at 11:24 AM,  <philippedeswert at gmail.com> wrote:
> From: Philippe De Swert <philippedeswert at gmail.com>
>
> hashmap_get can return null, so as we dereference it
> immediately after calling it, we could crash.
> It is unlikely to occur though I expect. I am however unsure
> what error should be reported (if at all).
>
> Coverity CID#1237656
> ---
>  src/resolve/resolved-manager.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/src/resolve/resolved-manager.c b/src/resolve/resolved-manager.c
> index f979897..ba175e6 100644
> --- a/src/resolve/resolved-manager.c
> +++ b/src/resolve/resolved-manager.c
> @@ -1695,6 +1695,8 @@ int manager_ifindex_is_loopback(Manager *m, int ifindex) {
>                  return -EINVAL;
>
>          l = hashmap_get(m->links, INT_TO_PTR(ifindex));
> +        if(!l)
> +                return -EINVAL;

Ok, this can really be triggered if we receive messages on a link that
got hotplugged. This is unlikely, but I'm not sure treating such links
as loopback is the way to go. Maybe Tom has an idea what to do?

The callers of manager_ifindex_is_loopback() treat errors as 1, thus
as loopback device. They lookup the device via address-match then, so
maybe this is fine. But I feel uncomfortable applying this without
mentioning the race anywhere. Calling into RTNL to read the link is
probably overkill.

We could set the RTNL event priority higher than the I/O socket, so
the RTNL socket is guaranteed to be read prior to any packet. That's
still not a guarantee, though. And maybe I care way too much for such
a tiny tiny race.. who knows.. I hope Tom can apply this.

Thanks
David

>          if (l->flags & IFF_LOOPBACK)
>                  return 1;
>
> --
> 1.8.3.2
>
> _______________________________________________
> systemd-devel mailing list
> systemd-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel