[libvirt PATCH] network: do not assume dnsmasq in networkUpdateState

[Michal Prívozník]

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 at redhat.com>
>> ---
>>  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);
     }


But this is broken by design. We let dnsmasq write the PID file and it
just so happens that dnsmasq doesn't keep it locked. So we play a
guessing game and check whether the pid we've read from the pidfile
belongs to a dnsmasq process. Any dnsmasq process, not just the one the
pidfile belongs to. We do the same with passt though, and when I pointed
that out in my review I was referred to this code.

Michal