If we don't have dnsmasq, it's pointless to try to find its pidfile. Also, dnsmasqCapsGetBinaryPath would access a NULL pointer. Fixes: 4b68c982e283471575bacbf87302495864da46fe Foxes: https://gitlab.com/libvirt/libvirt/-/issues/456

Signed-off-by: Ján Tomko <jtomko(a)redhat.com&gt; --- src/network/bridge_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 3fa56bfc09..ee4bbd4a93 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -492,7 +492,7 @@ networkUpdateState(virNetworkObj *obj, virNetworkObjPortForEach(obj, networkUpdatePort, obj); /* Try and read dnsmasq pids of active networks */ - if (virNetworkObjIsActive(obj) && def->ips && (def->nips > 0)) { + if (dnsmasq_caps && virNetworkObjIsActive(obj) && def->ips && (def->nips > 0)) { pid_t dnsmasqPid;

On 3/17/23 15:59, Peter Krempa wrote: > On Thu, Mar 16, 2023 at 14:21:27 +0100, Ján Tomko wrote: >> If we don't have dnsmasq, it's pointless to try to find >> its pidfile. >> >> Also, dnsmasqCapsGetBinaryPath would access a NULL pointer. >> >> Fixes: 4b68c982e283471575bacbf87302495864da46fe >> Foxes: https://gitlab.com/libvirt/libvirt/-/issues/456 > > Awww ^_^ > >> Signed-off-by: Ján Tomko <jtomko(a)redhat.com&gt; >> --- >> src/network/bridge_driver.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c >> index 3fa56bfc09..ee4bbd4a93 100644 >> --- a/src/network/bridge_driver.c >> +++ b/src/network/bridge_driver.c >> @@ -492,7 +492,7 @@ networkUpdateState(virNetworkObj *obj, >> virNetworkObjPortForEach(obj, networkUpdatePort, obj); >> >> /* Try and read dnsmasq pids of active networks */ >> - if (virNetworkObjIsActive(obj) && def->ips && (def->nips > 0)) { >> + if (dnsmasq_caps && virNetworkObjIsActive(obj) && def->ips && (def->nips > 0)) { >> pid_t dnsmasqPid; > > Based on the fact that at the beginning of this function we check: > > if (!virNetworkObjIsActive(obj)) > return 0; > > If we get to this place and don't have the capabilities this must mean > that the 'dnsmasq' binary was removed during runtime, right? > > In such case we should still be able to read the pidfile though as the > process is supposed to be around. > Yeah, for this particular bug we may need to go with: diff --git i/src/network/bridge_driver.c w/src/network/bridge_driver.c index 3fa56bfc09..a11a53501f 100644 --- i/src/network/bridge_driver.c +++ w/src/network/bridge_driver.c @@ -493,15 +493,19 @@ networkUpdateState(virNetworkObj *obj, /* Try and read dnsmasq pids of active networks */ if (virNetworkObjIsActive(obj) && def->ips && (def->nips > 0)) { + const char *binpath = NULL; pid_t dnsmasqPid; if (networkSetMacMap(cfg, obj) < 0) return -1; + if (dnsmasq_caps) + binpath = dnsmasqCapsGetBinaryPath(dnsmasq_caps); + ignore_value(virPidFileReadIfAlive(cfg->pidDir, def->name, &dnsmasqPid, - dnsmasqCapsGetBinaryPath(dnsmasq_caps))); + binpath)); virNetworkObjSetDnsmasqPid(obj, dnsmasqPid); }

On a Thursday in 2023, Michal Prívozník wrote: > On 3/22/23 11:07, Michal Prívozník wrote: >> On 3/17/23 15:59, Peter Krempa wrote: >>> On Thu, Mar 16, 2023 at 14:21:27 +0100, Ján Tomko wrote: >>>> If we don't have dnsmasq, it's pointless to try to find >>>> its pidfile. >>>> >>>> Also, dnsmasqCapsGetBinaryPath would access a NULL pointer. >>>> >>>> Fixes: 4b68c982e283471575bacbf87302495864da46fe >>>> Foxes: https://gitlab.com/libvirt/libvirt/-/issues/456 >>> >>> Awww ^_^ >>> >>>> Signed-off-by: Ján Tomko <jtomko(a)redhat.com&gt; >>>> --- >>>>  src/network/bridge_driver.c | 2 +- >>>>  1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c >>>> index 3fa56bfc09..ee4bbd4a93 100644 >>>> --- a/src/network/bridge_driver.c >>>> +++ b/src/network/bridge_driver.c >>>> @@ -492,7 +492,7 @@ networkUpdateState(virNetworkObj *obj, >>>>      virNetworkObjPortForEach(obj, networkUpdatePort, obj); >>>> >>>>      /* Try and read dnsmasq pids of active networks */ >>>> -    if (virNetworkObjIsActive(obj) && def->ips && (def->nips > 0)) { >>>> +    if (dnsmasq_caps && virNetworkObjIsActive(obj) && def->ips && >>>> (def->nips > 0)) { >>>>          pid_t dnsmasqPid; >>> >>> Based on the fact that at the beginning of this function we check: >>> >>>  if (!virNetworkObjIsActive(obj)) >>>      return 0; >>> >>> If we get to this place and don't have the capabilities this must mean >>> that the 'dnsmasq' binary was removed during runtime, right? >>> >>> In such case we should still be able to read the pidfile though as the >>> process is supposed to be around. >>> >> >> Yeah, for this particular bug we may need to go with: >> >> diff --git i/src/network/bridge_driver.c w/src/network/bridge_driver.c >> index 3fa56bfc09..a11a53501f 100644 >> --- i/src/network/bridge_driver.c >> +++ w/src/network/bridge_driver.c >> @@ -493,15 +493,19 @@ networkUpdateState(virNetworkObj *obj, >> >>      /* Try and read dnsmasq pids of active networks */ >>      if (virNetworkObjIsActive(obj) && def->ips && (def->nips > 0)) { >> +        const char *binpath = NULL; >>          pid_t dnsmasqPid; >> >>          if (networkSetMacMap(cfg, obj) < 0) >>              return -1; >> >> +        if (dnsmasq_caps) >> +            binpath = dnsmasqCapsGetBinaryPath(dnsmasq_caps); >> + >>          ignore_value(virPidFileReadIfAlive(cfg->pidDir, >>                                             def->name, >>                                             &dnsmasqPid, >> -                                           >> dnsmasqCapsGetBinaryPath(dnsmasq_caps))); >> +                                           binpath)); >>          virNetworkObjSetDnsmasqPid(obj, dnsmasqPid); >>      } >> >> > > Jano, you do want to resubmit the patch? > I do not. Going with your proposed changes, we'd lose the pid reuse protection

and I do not have the energy to rewrite how we deal with dnsmasq pidfiles.