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@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;
if (networkSetMacMap(cfg, obj) < 0)
--
2.39.2
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@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.
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@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
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@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);
> }
>
>
Jano, you do want to resubmit the patch?
Michal
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@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);
>> }
>>
>>
>
>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.
Jano
>Michal
>
On 4/13/23 17:22, Ján Tomko wrote:
> 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@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);
>>> }
>>>
>>>
>>
>> Jano, you do want to resubmit the patch?
>>
>
> I do not.
>
> Going with your proposed changes, we'd lose the pid reuse protection
Which is broken anyway. I mean, since the PID file isn't locked, then we
just rely on the fact that PID we've read is that of just any dnsmasq
process. Not the one that corresponds to this network, ANY dnsmasq process.
BTW: the commit you claim broke this - we didn't have this PID
"protection" even before that. Because before that commit, the dnsmasq
bin path was:
caps ? caps->binaryPath : DNSMASQ;
where this DNSMASQ macro is just a "dnsmasq" string and
virPidFileReadPathIfAlive() is not written in a way that would accept
non-absolute binPath.
> and
> I do not have the energy to rewrite how we deal with dnsmasq pidfiles.
Understandable and relatable.
Michal
© 2016 - 2024 Red Hat, Inc.