v4 of:

https://listman.redhat.com/archives/libvir-list/2022-January/msg00498.html

diff to v3:
- Removed even more code
- Switched from virCommandDryRun to a python script, per Andrea's
  request
- Reordered patches so that the mock doesn't need to redirect stat()
- Renamed mock
- Removed exec() of 'dnsmasq --help'

Michal Prívozník (13):
  virdnsmasq: Drop @binaryPath argument from dnsmasqCapsNewEmpty()
  lib: Prefer g_autoptr(dnsmasqCaps) instead of explicit unref
  virdnsmasq: Drop @force argument of dnsmasqCapsRefreshInternal()
  virdnsmasq: Drop mtime member from struct _dnsmasqCaps
  virdnsmasq: Drop noRefresh member from from struct _dnsmasqCaps
  virdnsmasq: Drop !caps check from dnsmasqCapsRefreshInternal()
  virdnsmasq: Don't run 'dnsmasq --help'
  virdnsmasq: Lookup DNSMASQ in PATH
  virdnsmasq: Require non NULL @caps in dnsmasqCapsGetBinaryPath()
  networkxml2conftest: Use dnsmasqCapsNewFromBinary() to construct caps
  networkxml2conftest: Check if capabilities were created successfully
  virdnsmasq: Drop dnsmasqCapsNewFromBuffer()
  virdnsmasq: Join dnsmasqCapsNewEmpty() and dnsmasqCapsNewFromBinary()

 meson.build                 |  1 -
 src/libvirt_private.syms    |  1 -
 src/util/virdnsmasq.c       | 84 ++++++-------------------------------
 src/util/virdnsmasq.h       |  1 -
 tests/dnsmasqmock.py        | 13 ++++++
 tests/meson.build           |  1 +
 tests/networkxml2conftest.c | 13 +++---
 tests/virdnsmasqmock.c      | 19 +++++++++
 8 files changed, 54 insertions(+), 79 deletions(-)
 create mode 100755 tests/dnsmasqmock.py
 create mode 100644 tests/virdnsmasqmock.c

-- 
2.34.1

On Mon, Jan 17, 2022 at 04:19:14PM +0100, Michal Privoznik wrote:
> v4 of:
>
> https://listman.redhat.com/archives/libvir-list/2022-January/msg00498.html
>
> diff to v3:
> - Removed even more code
> - Switched from virCommandDryRun to a python script, per Andrea's
>   request
> - Reordered patches so that the mock doesn't need to redirect stat()
> - Renamed mock
> - Removed exec() of 'dnsmasq --help'
>
> Michal Prívozník (13):
>   virdnsmasq: Drop @binaryPath argument from dnsmasqCapsNewEmpty()
>   lib: Prefer g_autoptr(dnsmasqCaps) instead of explicit unref
>   virdnsmasq: Drop @force argument of dnsmasqCapsRefreshInternal()
>   virdnsmasq: Drop mtime member from struct _dnsmasqCaps
>   virdnsmasq: Drop noRefresh member from from struct _dnsmasqCaps
>   virdnsmasq: Drop !caps check from dnsmasqCapsRefreshInternal()
>   virdnsmasq: Don't run 'dnsmasq --help'
>   virdnsmasq: Lookup DNSMASQ in PATH
>   virdnsmasq: Require non NULL @caps in dnsmasqCapsGetBinaryPath()
>   networkxml2conftest: Use dnsmasqCapsNewFromBinary() to construct caps
>   networkxml2conftest: Check if capabilities were created successfully
>   virdnsmasq: Drop dnsmasqCapsNewFromBuffer()
>   virdnsmasq: Join dnsmasqCapsNewEmpty() and dnsmasqCapsNewFromBinary()

Reviewed-by: Andrea Bolognani <abologna@redhat.com>

Thanks for bearing with me!

-- 
Andrea Bolognani / Red Hat / Virtualization

On Mon, Jan 17, 2022 at 08:24:36AM -0800, Andrea Bolognani wrote:
> On Mon, Jan 17, 2022 at 04:19:14PM +0100, Michal Privoznik wrote:
> > Michal Prívozník (13):
> >   virdnsmasq: Drop @binaryPath argument from dnsmasqCapsNewEmpty()
> >   lib: Prefer g_autoptr(dnsmasqCaps) instead of explicit unref
> >   virdnsmasq: Drop @force argument of dnsmasqCapsRefreshInternal()
> >   virdnsmasq: Drop mtime member from struct _dnsmasqCaps
> >   virdnsmasq: Drop noRefresh member from from struct _dnsmasqCaps
> >   virdnsmasq: Drop !caps check from dnsmasqCapsRefreshInternal()
> >   virdnsmasq: Don't run 'dnsmasq --help'
> >   virdnsmasq: Lookup DNSMASQ in PATH
> >   virdnsmasq: Require non NULL @caps in dnsmasqCapsGetBinaryPath()
> >   networkxml2conftest: Use dnsmasqCapsNewFromBinary() to construct caps
> >   networkxml2conftest: Check if capabilities were created successfully
> >   virdnsmasq: Drop dnsmasqCapsNewFromBuffer()
> >   virdnsmasq: Join dnsmasqCapsNewEmpty() and dnsmasqCapsNewFromBinary()
>
> Reviewed-by: Andrea Bolognani <abologna@redhat.com>

These changes seem to have made ASAN very unhappy, see

  https://gitlab.com/MichalPrivoznik/libvirt/-/jobs/1985244739
  https://gitlab.com/MichalPrivoznik/libvirt/-/jobs/1985244740

Tim, do you have any idea why that would be the case? My uneducated
guess is that the environment needed by ASAN is somehow lost when the
dnsmasqmock.py script is called, but I'm unfamiliar with how these
tools actually work.

-- 
Andrea Bolognani / Red Hat / Virtualization

On 1/18/22 11:14, Andrea Bolognani wrote:
> On Mon, Jan 17, 2022 at 08:24:36AM -0800, Andrea Bolognani wrote:
>> On Mon, Jan 17, 2022 at 04:19:14PM +0100, Michal Privoznik wrote:
>>> Michal Prívozník (13):
>>>   virdnsmasq: Drop @binaryPath argument from dnsmasqCapsNewEmpty()
>>>   lib: Prefer g_autoptr(dnsmasqCaps) instead of explicit unref
>>>   virdnsmasq: Drop @force argument of dnsmasqCapsRefreshInternal()
>>>   virdnsmasq: Drop mtime member from struct _dnsmasqCaps
>>>   virdnsmasq: Drop noRefresh member from from struct _dnsmasqCaps
>>>   virdnsmasq: Drop !caps check from dnsmasqCapsRefreshInternal()
>>>   virdnsmasq: Don't run 'dnsmasq --help'
>>>   virdnsmasq: Lookup DNSMASQ in PATH
>>>   virdnsmasq: Require non NULL @caps in dnsmasqCapsGetBinaryPath()
>>>   networkxml2conftest: Use dnsmasqCapsNewFromBinary() to construct caps
>>>   networkxml2conftest: Check if capabilities were created successfully
>>>   virdnsmasq: Drop dnsmasqCapsNewFromBuffer()
>>>   virdnsmasq: Join dnsmasqCapsNewEmpty() and dnsmasqCapsNewFromBinary()
>>
>> Reviewed-by: Andrea Bolognani <abologna@redhat.com>
> 
> These changes seem to have made ASAN very unhappy, see
> 
>   https://gitlab.com/MichalPrivoznik/libvirt/-/jobs/1985244739
>   https://gitlab.com/MichalPrivoznik/libvirt/-/jobs/1985244740
> 
> Tim, do you have any idea why that would be the case? My uneducated
> guess is that the environment needed by ASAN is somehow lost when the
> dnsmasqmock.py script is called, but I'm unfamiliar with how these
> tools actually work.
> 

I might know what the problem is. When networkxml2conf script is called
the linker links asan and in its initializer it finds no problem. But
then main() of the test is called which modifies LD_PRELOAD (by putting
all mock libs of ours at the beginning) and re-exec()-s itself. After
that, mymain() is called which in turn calls dnsmasqCapsNewFromBinary()
to construct capabilities. Transitionally, the following piece of code
is executed:

    vercmd = virCommandNewArgList(caps->binaryPath, "--version", NULL);
    virCommandSetOutputBuffer(vercmd, &version);
    virCommandAddEnvPassCommon(vercmd);
    virCommandClearCaps(vercmd);
    if (virCommandRun(vercmd, NULL) < 0)


Here, virCommandAddEnvPassCommon() makes caps->binaryPath (=
tests/dnsmasqmock.py) run with LD_PRELOAD preserved. But at this point
the asan is not the first library on the list, our mocks are. Hence,
asan is unhappy.

The way this is resolved in other tests (e.g. qemuxml2argvtest) is by
using virfilewrapper.c. At the beginning of mymain() in
qemuxml2argvtest.c there are couple of calls to
virFileWrapperAddPrefix(), for instance:

    virFileWrapperAddPrefix("/usr/libexec/qemu/vhost-user",
                            abs_srcdir
"/qemuvhostuserdata/usr/libexec/qemu/vhost-user");


Which means that later when our code wants to execute
/usr/libexec/qemu/vhost-user/$binary it actually executes
$abs_srcdir/qemuvhostuserdata/user/libexec/qemu/$binary.

Sweet. Except that won't fly for networkxml2conf test, will it. In case
of qemu we have control over binaries and paths we are executing, but
for dnsmasq we want to look the binary up in PATH. Having said that, I
could mock virFindFileInPath() just like I am now, except let it return
a predictable path (say /usr/sbin/dnsmasq) and then use
virFileWrapper...() to redirect /usr/sbin/ to abs_srcdir.

Alternative to all of this is to keed virCommandSetDryRun() just like I
had in one of previous patches. Remind me please, what was the issue
with that?

Michal

On Tue, Jan 18, 2022 at 12:30:30PM +0100, Michal Prívozník wrote:
> On 1/18/22 11:14, Andrea Bolognani wrote:
> > These changes seem to have made ASAN very unhappy, see
> >
> >   https://gitlab.com/MichalPrivoznik/libvirt/-/jobs/1985244739
> >   https://gitlab.com/MichalPrivoznik/libvirt/-/jobs/1985244740
> >
> > Tim, do you have any idea why that would be the case? My uneducated
> > guess is that the environment needed by ASAN is somehow lost when the
> > dnsmasqmock.py script is called, but I'm unfamiliar with how these
> > tools actually work.
>
> [...] I
> could mock virFindFileInPath() just like I am now, except let it return
> a predictable path (say /usr/sbin/dnsmasq) and then use
> virFileWrapper...() to redirect /usr/sbin/ to abs_srcdir.
>
> Alternative to all of this is to keed virCommandSetDryRun() just like I
> had in one of previous patches. Remind me please, what was the issue
> with that?

The Python script approach seemed simpler, but in light of this issue
I guess that argument has gone completely out of the window :)

Can you please try a version of this series with your original
dnsmasq mocking approach in CI and see whether ASAN is happy with it?
If so, we can just go ahead with that one.

-- 
Andrea Bolognani / Red Hat / Virtualization

On 1/18/22 13:57, Andrea Bolognani wrote:
> On Tue, Jan 18, 2022 at 12:30:30PM +0100, Michal Prívozník wrote:
>> On 1/18/22 11:14, Andrea Bolognani wrote:
>>> These changes seem to have made ASAN very unhappy, see
>>>
>>>   https://gitlab.com/MichalPrivoznik/libvirt/-/jobs/1985244739
>>>   https://gitlab.com/MichalPrivoznik/libvirt/-/jobs/1985244740
>>>
>>> Tim, do you have any idea why that would be the case? My uneducated
>>> guess is that the environment needed by ASAN is somehow lost when the
>>> dnsmasqmock.py script is called, but I'm unfamiliar with how these
>>> tools actually work.
>>
>> [...] I
>> could mock virFindFileInPath() just like I am now, except let it return
>> a predictable path (say /usr/sbin/dnsmasq) and then use
>> virFileWrapper...() to redirect /usr/sbin/ to abs_srcdir.
>>
>> Alternative to all of this is to keed virCommandSetDryRun() just like I
>> had in one of previous patches. Remind me please, what was the issue
>> with that?
> 
> The Python script approach seemed simpler, but in light of this issue
> I guess that argument has gone completely out of the window :)
> 
> Can you please try a version of this series with your original
> dnsmasq mocking approach in CI and see whether ASAN is happy with it?
> If so, we can just go ahead with that one.
> 

Will do. Although, since virCommandRun() wouldn't actually execute
anything I don't expect ASAN to raise any issues. Meanwhile, I'm testing
the approach I've outlined:

https://gitlab.com/MichalPrivoznik/libvirt/-/pipelines/450382098

Let's see how it runs.

Michal

On 1/18/22 14:05, Michal Prívozník wrote:
> On 1/18/22 13:57, Andrea Bolognani wrote:
>> On Tue, Jan 18, 2022 at 12:30:30PM +0100, Michal Prívozník wrote:
>>> On 1/18/22 11:14, Andrea Bolognani wrote:
>>>> These changes seem to have made ASAN very unhappy, see
>>>>
>>>>   https://gitlab.com/MichalPrivoznik/libvirt/-/jobs/1985244739
>>>>   https://gitlab.com/MichalPrivoznik/libvirt/-/jobs/1985244740
>>>>
>>>> Tim, do you have any idea why that would be the case? My uneducated
>>>> guess is that the environment needed by ASAN is somehow lost when the
>>>> dnsmasqmock.py script is called, but I'm unfamiliar with how these
>>>> tools actually work.
>>>
>>> [...] I
>>> could mock virFindFileInPath() just like I am now, except let it return
>>> a predictable path (say /usr/sbin/dnsmasq) and then use
>>> virFileWrapper...() to redirect /usr/sbin/ to abs_srcdir.
>>>
>>> Alternative to all of this is to keed virCommandSetDryRun() just like I
>>> had in one of previous patches. Remind me please, what was the issue
>>> with that?
>>
>> The Python script approach seemed simpler, but in light of this issue
>> I guess that argument has gone completely out of the window :)
>>
>> Can you please try a version of this series with your original
>> dnsmasq mocking approach in CI and see whether ASAN is happy with it?
>> If so, we can just go ahead with that one.
>>
> 
> Will do. Although, since virCommandRun() wouldn't actually execute
> anything I don't expect ASAN to raise any issues. Meanwhile, I'm testing
> the approach I've outlined:
> 
> https://gitlab.com/MichalPrivoznik/libvirt/-/pipelines/450382098
> 
> Let's see how it runs.

Aaand I have the results:

failed to create the fake capabilities: internal error: Child process
(LC_ALL=C
LD_PRELOAD=/builds/MichalPrivoznik/libvirt/build/tests/libvirdnsmasqmock.so
PATH=/builds/MichalPrivoznik/libvirt/build/tests:/usr/libexec/ccache-wrappers:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
HOME=/bad-test-used-env-home /usr/local/sbin/dnsmasqmock.py --version)
unexpected exit status 127: /usr/bin/env: symbol lookup error:
/builds/MichalPrivoznik/libvirt/build/tests/../src/libvirt.so.0:
undefined symbol: __asan_option_detect_stack_use_after_return

Why does dnsmasqmock.py try to link with libvirt.so.0 or why there's a
missing symbol is beyond me. So let me just stick with what I suggested
initially.

Michal

On Tue, Jan 18, 2022 at 02:13:23PM +0100, Michal Prívozník wrote:
> On 1/18/22 14:05, Michal Prívozník wrote:
> > On 1/18/22 13:57, Andrea Bolognani wrote:
> >> Can you please try a version of this series with your original
> >> dnsmasq mocking approach in CI and see whether ASAN is happy with it?
> >> If so, we can just go ahead with that one.
> >
> > Will do. Although, since virCommandRun() wouldn't actually execute
> > anything I don't expect ASAN to raise any issues. Meanwhile, I'm testing
> > the approach I've outlined:
> >
> > https://gitlab.com/MichalPrivoznik/libvirt/-/pipelines/450382098
> >
> > Let's see how it runs.
>
> Aaand I have the results:
>
> failed to create the fake capabilities: internal error: Child process
> (LC_ALL=C
> LD_PRELOAD=/builds/MichalPrivoznik/libvirt/build/tests/libvirdnsmasqmock.so
> PATH=/builds/MichalPrivoznik/libvirt/build/tests:/usr/libexec/ccache-wrappers:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
> HOME=/bad-test-used-env-home /usr/local/sbin/dnsmasqmock.py --version)
> unexpected exit status 127: /usr/bin/env: symbol lookup error:
> /builds/MichalPrivoznik/libvirt/build/tests/../src/libvirt.so.0:
> undefined symbol: __asan_option_detect_stack_use_after_return
>
> Why does dnsmasqmock.py try to link with libvirt.so.0 or why there's a
> missing symbol is beyond me. So let me just stick with what I suggested
> initially.

Yeah, that sounds good. Can you please make sure the result passes CI
and post it to the list? IIRC you've shuffled patches around in the
meantime, so we should do one last quick sanity check before pushing.

-- 
Andrea Bolognani / Red Hat / Virtualization

On 1/18/22 15:08, Andrea Bolognani wrote:
> On Tue, Jan 18, 2022 at 02:13:23PM +0100, Michal Prívozník wrote:
>> On 1/18/22 14:05, Michal Prívozník wrote:
>>> On 1/18/22 13:57, Andrea Bolognani wrote:
>>>> Can you please try a version of this series with your original
>>>> dnsmasq mocking approach in CI and see whether ASAN is happy with it?
>>>> If so, we can just go ahead with that one.
>>>
>>> Will do. Although, since virCommandRun() wouldn't actually execute
>>> anything I don't expect ASAN to raise any issues. Meanwhile, I'm testing
>>> the approach I've outlined:
>>>
>>> https://gitlab.com/MichalPrivoznik/libvirt/-/pipelines/450382098
>>>
>>> Let's see how it runs.
>>
>> Aaand I have the results:
>>
>> failed to create the fake capabilities: internal error: Child process
>> (LC_ALL=C
>> LD_PRELOAD=/builds/MichalPrivoznik/libvirt/build/tests/libvirdnsmasqmock.so
>> PATH=/builds/MichalPrivoznik/libvirt/build/tests:/usr/libexec/ccache-wrappers:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
>> HOME=/bad-test-used-env-home /usr/local/sbin/dnsmasqmock.py --version)
>> unexpected exit status 127: /usr/bin/env: symbol lookup error:
>> /builds/MichalPrivoznik/libvirt/build/tests/../src/libvirt.so.0:
>> undefined symbol: __asan_option_detect_stack_use_after_return
>>
>> Why does dnsmasqmock.py try to link with libvirt.so.0 or why there's a
>> missing symbol is beyond me. So let me just stick with what I suggested
>> initially.
> 
> Yeah, that sounds good. Can you please make sure the result passes CI
> and post it to the list? IIRC you've shuffled patches around in the
> meantime, so we should do one last quick sanity check before pushing.
> 

Alright, although the only change I did really was in the patch 10/13
which I replaced with the corresponding patch from my earlier version.
But anyway, let's respin another version (hopefully the last).

The pipeline's green:

https://gitlab.com/MichalPrivoznik/libvirt/-/pipelines/450418184

Michal