[PATCH v4 00/13] virdnsmasq: Lookup DNSMASQ in PATH

[Michal Prívozník]

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 at 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