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

[Tom Gundersen]

On Sat, Sep 13, 2014 at 12:10 PM, David Herrmann <dh.herrmann at gmail.com> wrote:
> 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?

Yeah, this could happen. It so happens that the loopback link will
always have ifindex 1, so I guess we could just fall back to checking
for that if we don't have the real flags.

> The callers of manager_ifindex_is_loopback() treat errors as 1, thus
> as loopback device.

Hm, yeah, this might be fine, but it feels wrong. I mean we will
_always_ have the real loopback device in the hash (as it cannot be
hotplugged), so the only time this would fail it would be for a
non-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.

It might make sense to make sure RTNL has the highest priority, but
I'll leave that to Lennart, in the meantime I'll added the fallback to
checking the ifindex.

Thanks for the report!

Cheers,

Tom