On Sat, Sep 13, 2014 at 12:10 PM, David Herrmann <[email protected]> wrote: > Hi > > On Sat, Sep 13, 2014 at 11:24 AM, <[email protected]> wrote: >> From: Philippe De Swert <[email protected]> >> >> 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 _______________________________________________ systemd-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/systemd-devel
