[libvirt] [PATCH v3 0/3] Don't touch user data from tests
v3 of: https://www.redhat.com/archives/libvir-list/2018-July/msg00747.html diff to v2: - pushed some already ACKed patches - dropped controversial line from 1/3 that turned off error reporting Michal Prívozník (3): qemuxml2argvtest: Set more fake drivers check-file-access: Allow specifying action virtestmock: Track action tests/check-file-access.pl | 32 +++++++++++++++++++++++++++----- tests/file_access_whitelist.txt | 15 ++++++++++----- tests/qemuxml2argvtest.c | 4 ++++ tests/virtestmock.c | 39 ++++++++++++++++++++++----------------- 4 files changed, 63 insertions(+), 27 deletions(-) -- 2.16.4
So far we are setting only fake secret and storage drivers. Therefore if the code wants to call a public NWFilter API (like qemuBuildInterfaceCommandLine() and qemuBuildNetCommandLine() are doing) the virGetConnectNWFilter() function will try to actually spawn session daemon because there's no connection object set to handle NWFilter driver. Even though I haven't experienced the same problem with the rest of the drivers (interface, network and node dev), the reasoning above can be applied to them as well. At the same time, now that connection object is registered for the drivers, the public APIs will throw virReportUnsupportedError(). And since we don't provide any error func the error is printed to stderr. Fix this by setting dummy error func. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/qemuxml2argvtest.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 84117a3e63..8901c7bde4 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -471,6 +471,10 @@ testCompareXMLToArgv(const void *data) conn->secretDriver = &fakeSecretDriver; conn->storageDriver = &fakeStorageDriver; + virSetConnectInterface(conn); + virSetConnectNetwork(conn); + virSetConnectNWFilter(conn); + virSetConnectNodeDev(conn); virSetConnectSecret(conn); virSetConnectStorage(conn); -- 2.16.4
On Fri, Jul 27, 2018 at 05:24:45PM +0200, Michal Privoznik wrote:
So far we are setting only fake secret and storage drivers. Therefore if the code wants to call a public NWFilter API (like qemuBuildInterfaceCommandLine() and qemuBuildNetCommandLine() are doing) the virGetConnectNWFilter() function will try to actually spawn session daemon because there's no connection object set to handle NWFilter driver.
Even though I haven't experienced the same problem with the rest of the drivers (interface, network and node dev), the reasoning above can be applied to them as well.
At the same time, now that connection object is registered for the drivers, the public APIs will throw virReportUnsupportedError(). And since we don't provide any error func the error is printed to stderr. Fix this by setting dummy error func.
Is this last paragraph stale ? you're not seeing a dummy error func in this patch ...
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/qemuxml2argvtest.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 84117a3e63..8901c7bde4 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -471,6 +471,10 @@ testCompareXMLToArgv(const void *data) conn->secretDriver = &fakeSecretDriver; conn->storageDriver = &fakeStorageDriver;
+ virSetConnectInterface(conn); + virSetConnectNetwork(conn); + virSetConnectNWFilter(conn); + virSetConnectNodeDev(conn); virSetConnectSecret(conn); virSetConnectStorage(conn);
-- 2.16.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 07/27/2018 05:29 PM, Daniel P. Berrangé wrote:
On Fri, Jul 27, 2018 at 05:24:45PM +0200, Michal Privoznik wrote:
So far we are setting only fake secret and storage drivers. Therefore if the code wants to call a public NWFilter API (like qemuBuildInterfaceCommandLine() and qemuBuildNetCommandLine() are doing) the virGetConnectNWFilter() function will try to actually spawn session daemon because there's no connection object set to handle NWFilter driver.
Even though I haven't experienced the same problem with the rest of the drivers (interface, network and node dev), the reasoning above can be applied to them as well.
At the same time, now that connection object is registered for the drivers, the public APIs will throw virReportUnsupportedError(). And since we don't provide any error func the error is printed to stderr. Fix this by setting dummy error func.
Is this last paragraph stale ? you're not seeing a dummy error func in this patch ...
Oh yes, I am: libvirt.git/tests $ VIR_TEST_DEBUG=1 ./qemuxml2argvtest 2>&1 | grep "this function is not supported" 441) QEMU XML-2-ARGV net-vhostuser-multiq ... libvirt: Network Filter Driver error : this function is not supported by the connection driver: virNWFilterBindingLookupByPortDev libvirt: Network Filter Driver error : this function is not supported by the connection driver: virNWFilterBindingLookupByPortDev libvirt: Network Filter Driver error : this function is not supported by the connection driver: virNWFilterBindingLookupByPortDev 2018-07-27 16:02:50.689+0000: 11166: error : virNWFilterBindingLookupByPortDev:599 : this function is not supported by the connection driver: virNWFilterBindingLookupByPortDev 2018-07-27 16:02:50.689+0000: 11166: error : virNWFilterBindingLookupByPortDev:599 : this function is not supported by the connection driver: virNWFilterBindingLookupByPortDev 2018-07-27 16:02:50.689+0000: 11166: error : virNWFilterBindingLookupByPortDev:599 : this function is not supported by the connection driver: virNWFilterBindingLookupByPortDev (Or even without DEBUG) But as I told Peter, at this point stopping touching live user data is more important to me than silencing some error message. Michal
On 07/27/2018 12:06 PM, Michal Privoznik wrote:
On 07/27/2018 05:29 PM, Daniel P. Berrangé wrote:
On Fri, Jul 27, 2018 at 05:24:45PM +0200, Michal Privoznik wrote:
So far we are setting only fake secret and storage drivers. Therefore if the code wants to call a public NWFilter API (like qemuBuildInterfaceCommandLine() and qemuBuildNetCommandLine() are doing) the virGetConnectNWFilter() function will try to actually spawn session daemon because there's no connection object set to handle NWFilter driver.
Even though I haven't experienced the same problem with the rest of the drivers (interface, network and node dev), the reasoning above can be applied to them as well.
At the same time, now that connection object is registered for the drivers, the public APIs will throw virReportUnsupportedError(). And since we don't provide any error func the error is printed to stderr. Fix this by setting dummy error func.
Is this last paragraph stale ? you're not seeing a dummy error func in this patch ...
Oh yes, I am:
libvirt.git/tests $ VIR_TEST_DEBUG=1 ./qemuxml2argvtest 2>&1 | grep "this function is not supported" 441) QEMU XML-2-ARGV net-vhostuser-multiq ... libvirt: Network Filter Driver error : this function is not supported by the connection driver: virNWFilterBindingLookupByPortDev libvirt: Network Filter Driver error : this function is not supported by the connection driver: virNWFilterBindingLookupByPortDev libvirt: Network Filter Driver error : this function is not supported by the connection driver: virNWFilterBindingLookupByPortDev 2018-07-27 16:02:50.689+0000: 11166: error : virNWFilterBindingLookupByPortDev:599 : this function is not supported by the connection driver: virNWFilterBindingLookupByPortDev 2018-07-27 16:02:50.689+0000: 11166: error : virNWFilterBindingLookupByPortDev:599 : this function is not supported by the connection driver: virNWFilterBindingLookupByPortDev 2018-07-27 16:02:50.689+0000: 11166: error : virNWFilterBindingLookupByPortDev:599 : this function is not supported by the connection driver: virNWFilterBindingLookupByPortDev
(Or even without DEBUG) But as I told Peter, at this point stopping touching live user data is more important to me than silencing some error message.
The above error is only seen when the virSetConnectNWFilter is used... So, let's take the extra step to be more like fakeSecretDriver and fakeStorageDriver. Squash the following in and you can remove that last paragraph: diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index b905cc943c..cedef5c9ad 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -262,6 +262,33 @@ static virStorageDriver fakeStorageDriver = { .storagePoolIsActive = fakeStoragePoolIsActive, }; + +/* virNetDevOpenvswitchGetVhostuserIfname mocks a portdev name - handle that */ +static virNWFilterBindingPtr +fakeNWFilterBindingLookupByPortDev(virConnectPtr conn, + const char *portdev) +{ + if (STREQ(portdev, "vhost-user0")) + return virGetNWFilterBinding(conn, "fake_vnet0", "fakeFilterName"); + + virReportError(VIR_ERR_NO_NWFILTER_BINDING, + "no nwfilter binding for port dev '%s'", portdev); + return NULL; +} + + +static int +fakeNWFilterBindingDelete(virNWFilterBindingPtr binding ATTRIBUTE_UNUSED) +{ + return 0; +} + + +static virNWFilterDriver fakeNWFilterDriver = { + .nwfilterBindingLookupByPortDev = fakeNWFilterBindingLookupByPortDev, + .nwfilterBindingDelete = fakeNWFilterBindingDelete, +}; + typedef enum { FLAG_EXPECT_FAILURE = 1 << 0, FLAG_EXPECT_PARSE_ERROR = 1 << 1, @@ -470,6 +497,7 @@ testCompareXMLToArgv(const void *data) conn->secretDriver = &fakeSecretDriver; conn->storageDriver = &fakeStorageDriver; + conn->nwfilterDriver = &fakeNWFilterDriver; virSetConnectInterface(conn); virSetConnectNetwork(conn); ------------ What's really interesting (perhaps to me) is that as long as this patch (and the squashed hunk) are there, the error message you're trying to avoid in the next two patches no longer appears. That is - if just applying this patch, I don't see the Failed to connect socket to '/run/user/1000...'. If I comment out virSetConnectNWFilter, then the message reappears. If I flip-flop the patches (eg last 2 first) and rebuild, I don't see the changed error message. But that could be because some file isn't being rebuilt - I forget I think I tripped across it before, but cannot remember how. I did at least figure out why virSetConnectNWFilter triggers the calls you're trying to avoid from the commit message for/from the net-vhostuser-multiq. If qemuBuildVhostuserCommandLine fails, then we goto cleanup which calls virDomainConfNWFilterTeardown sending us down the path to calling virDomainConfNWFilterTeardownImpl because @conn is no longer NULL. This calls in succession virNWFilterBindingLookupByPortDev and virNWFilterBindingDelete before returning. It's still not clear to me why when using virSetConnectNWFilter does the "Failed to connect socket..." message not appear and I don't have the energy to chase that right now. So of course this is a really long way to say, I'm wondering whether the subsequent patches are really necessary or not. I'll leave that to you to decide. In any case, with the squashed bit applied... Reviewed-by: John Ferlan <jferlan@redhat.com> John
The check-file-access.pl script is used to match access list generated by virtestmock against whitelisted rules stored in file_access_whitelist.txt. So far the rules are in form: $path: $progname: $testname This is not sufficient because the rule does not take into account 'action' that caused $path to appear in the list of accessed files. After this commit the rule can be in new form: $path: $action: $progname: $testname where $action is one from ("open", "fopen", "access", "stat", "lstat", "connect"). This way the white list can be fine tuned to allow say access() but not connect(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/check-file-access.pl | 32 +++++++++++++++++++++++++++----- tests/file_access_whitelist.txt | 15 ++++++++++----- 2 files changed, 37 insertions(+), 10 deletions(-) diff --git a/tests/check-file-access.pl b/tests/check-file-access.pl index 977a2bc533..ea0b7a18a2 100755 --- a/tests/check-file-access.pl +++ b/tests/check-file-access.pl @@ -27,18 +27,21 @@ use warnings; my $access_file = "test_file_access.txt"; my $whitelist_file = "file_access_whitelist.txt"; +my @known_actions = ("open", "fopen", "access", "stat", "lstat", "connect"); + my @files; my @whitelist; open FILE, "<", $access_file or die "Unable to open $access_file: $!"; while (<FILE>) { chomp; - if (/^(\S*):\s*(\S*)(\s*:\s*(.*))?$/) { + if (/^(\S*):\s*(\S*):\s*(\S*)(\s*:\s*(.*))?$/) { my %rec; ${rec}{path} = $1; - ${rec}{progname} = $2; - if (defined $4) { - ${rec}{testname} = $4; + ${rec}{action} = $2; + ${rec}{progname} = $3; + if (defined $5) { + ${rec}{testname} = $5; } push (@files, \%rec); } else { @@ -52,7 +55,21 @@ while (<FILE>) { chomp; if (/^\s*#.*$/) { # comment + } elsif (/^(\S*):\s*(\S*)(:\s*(\S*)(\s*:\s*(.*))?)?$/ and + grep /^$2$/, @known_actions) { + # $path: $action: $progname: $testname + my %rec; + ${rec}{path} = $1; + ${rec}{action} = $3; + if (defined $4) { + ${rec}{progname} = $4; + } + if (defined $6) { + ${rec}{testname} = $6; + } + push (@whitelist, \%rec); } elsif (/^(\S*)(:\s*(\S*)(\s*:\s*(.*))?)?$/) { + # $path: $progname: $testname my %rec; ${rec}{path} = $1; if (defined $3) { @@ -79,6 +96,11 @@ for my $file (@files) { next; } + if (defined %${rule}{action} and + not %${file}{action} =~ m/^$rule->{action}$/) { + next; + } + if (defined %${rule}{progname} and not %${file}{progname} =~ m/^$rule->{progname}$/) { next; @@ -95,7 +117,7 @@ for my $file (@files) { if (not $match) { $error = 1; - print "$file->{path}: $file->{progname}"; + print "$file->{path}: $file->{action}: $file->{progname}"; print ": $file->{testname}" if defined %${file}{testname}; print "\n"; } diff --git a/tests/file_access_whitelist.txt b/tests/file_access_whitelist.txt index 850b28506e..3fb318cbab 100644 --- a/tests/file_access_whitelist.txt +++ b/tests/file_access_whitelist.txt @@ -1,14 +1,17 @@ # This is a whitelist that allows accesses to files not in our # build directory nor source directory. The records are in the -# following format: +# following formats: # # $path: $progname: $testname +# $path: $action: $progname: $testname # -# All these three are evaluated as perl RE. So to allow /dev/sda -# and /dev/sdb, you can just '/dev/sd[a-b]', or to allow +# All these variables are evaluated as perl RE. So to allow +# /dev/sda and /dev/sdb, you can just '/dev/sd[a-b]', or to allow # /proc/$pid/status you can '/proc/\d+/status' and so on. -# Moreover, $progname and $testname can be empty, in which which -# case $path is allowed for all tests. +# Moreover, $action, $progname and $testname can be empty, in which +# which case $path is allowed for all tests. However, $action (if +# specified) must be one of "open", "fopen", "access", "stat", +# "lstat", "connect". /bin/cat: sysinfotest /bin/dirname: sysinfotest: x86 sysinfo @@ -19,5 +22,7 @@ /etc/hosts /proc/\d+/status +/etc/passwd: fopen + # This is just a dummy example, DO NOT USE IT LIKE THAT! .*: nonexistent-test-touching-everything -- 2.16.4
On 07/27/2018 11:24 AM, Michal Privoznik wrote:
The check-file-access.pl script is used to match access list generated by virtestmock against whitelisted rules stored in file_access_whitelist.txt. So far the rules are in form:
$path: $progname: $testname
This is not sufficient because the rule does not take into account 'action' that caused $path to appear in the list of accessed files. After this commit the rule can be in new form:
$path: $action: $progname: $testname
where $action is one from ("open", "fopen", "access", "stat", "lstat", "connect"). This way the white list can be fine tuned to allow say access() but not connect().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/check-file-access.pl | 32 +++++++++++++++++++++++++++----- tests/file_access_whitelist.txt | 15 ++++++++++----- 2 files changed, 37 insertions(+), 10 deletions(-)
I think based on the previous time through this and the explanation provided afterwards I am comfortable with the changes. Still it would be nice perhaps to alter the comments in file_access_whitelist.txt in order to describe the various settings like you replied here: https://www.redhat.com/archives/libvir-list/2018-July/msg01434.html starting with "The idea is to have two sets of rules:" and copying enough of that in order to provide an example in the comments so that someone who really didn't have the desire or cycles to read the perl script could actually write a reasonable rule. Knowing "how" or "when" to use may be a good idea. After patch 1 there's no longer an example in the qemuxml2argvtest output. Consider it a weak because my perl scripting and regex knowledge isn't the best... Reviewed-by: John Ferlan <jferlan@redhat.com> John
As advertised in the previous commit, we need the list of accessed files to also contain action that caused the $path to appear on the list. Not only this enables us to fine tune our white list rules it also helps us to see why $path is reported. For instance: /run/user/1000/libvirt/libvirt-sock: connect: qemuxml2argvtest: QEMU XML-2-ARGV net-vhostuser-multiq Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virtestmock.c | 39 ++++++++++++++++++++++----------------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/tests/virtestmock.c b/tests/virtestmock.c index 654af24a10..25aadf8aea 100644 --- a/tests/virtestmock.c +++ b/tests/virtestmock.c @@ -88,7 +88,8 @@ static void init_syms(void) } static void -printFile(const char *file) +printFile(const char *file, + const char *func) { FILE *fp; const char *testname = getenv("VIR_TEST_MOCK_TESTNAME"); @@ -116,9 +117,9 @@ printFile(const char *file) } /* Now append the following line into the output file: - * $file: $progname $testname */ + * $file: $progname: $func: $testname */ - fprintf(fp, "%s: %s", file, progname); + fprintf(fp, "%s: %s: %s", file, func, progname); if (testname) fprintf(fp, ": %s", testname); @@ -128,8 +129,12 @@ printFile(const char *file) fclose(fp); } +#define CHECK_PATH(path) \ + checkPath(path, __FUNCTION__) + static void -checkPath(const char *path) +checkPath(const char *path, + const char *func) { char *fullPath = NULL; char *relPath = NULL; @@ -160,7 +165,7 @@ checkPath(const char *path) if (!STRPREFIX(path, abs_topsrcdir) && !STRPREFIX(path, abs_topbuilddir)) { - printFile(path); + printFile(path, func); } VIR_FREE(crippledPath); @@ -180,7 +185,7 @@ int open(const char *path, int flags, ...) init_syms(); - checkPath(path); + CHECK_PATH(path); if (flags & O_CREAT) { va_list ap; @@ -199,7 +204,7 @@ FILE *fopen(const char *path, const char *mode) { init_syms(); - checkPath(path); + CHECK_PATH(path); return real_fopen(path, mode); } @@ -209,7 +214,7 @@ int access(const char *path, int mode) { init_syms(); - checkPath(path); + CHECK_PATH(path); return real_access(path, mode); } @@ -239,7 +244,7 @@ int stat(const char *path, struct stat *sb) { init_syms(); - checkPath(path); + checkPath(path, "stat"); return real_stat(path, sb); } @@ -250,7 +255,7 @@ int stat64(const char *path, struct stat64 *sb) { init_syms(); - checkPath(path); + checkPath(path, "stat"); return real_stat64(path, sb); } @@ -262,7 +267,7 @@ __xstat(int ver, const char *path, struct stat *sb) { init_syms(); - checkPath(path); + checkPath(path, "stat"); return real___xstat(ver, path, sb); } @@ -274,7 +279,7 @@ __xstat64(int ver, const char *path, struct stat64 *sb) { init_syms(); - checkPath(path); + checkPath(path, "stat"); return real___xstat64(ver, path, sb); } @@ -286,7 +291,7 @@ lstat(const char *path, struct stat *sb) { init_syms(); - checkPath(path); + checkPath(path, "lstat"); return real_lstat(path, sb); } @@ -298,7 +303,7 @@ lstat64(const char *path, struct stat64 *sb) { init_syms(); - checkPath(path); + checkPath(path, "lstat"); return real_lstat64(path, sb); } @@ -310,7 +315,7 @@ __lxstat(int ver, const char *path, struct stat *sb) { init_syms(); - checkPath(path); + checkPath(path, "lstat"); return real___lxstat(ver, path, sb); } @@ -322,7 +327,7 @@ __lxstat64(int ver, const char *path, struct stat64 *sb) { init_syms(); - checkPath(path); + checkPath(path, "lstat"); return real___lxstat64(ver, path, sb); } @@ -337,7 +342,7 @@ int connect(int sockfd, const struct sockaddr *addr, socklen_t addrlen) if (addrlen == sizeof(struct sockaddr_un)) { struct sockaddr_un *tmp = (struct sockaddr_un *) addr; if (tmp->sun_family == AF_UNIX) - checkPath(tmp->sun_path); + CHECK_PATH(tmp->sun_path); } #endif -- 2.16.4
On 07/27/2018 11:24 AM, Michal Privoznik wrote:
As advertised in the previous commit, we need the list of accessed files to also contain action that caused the $path to appear on the list. Not only this enables us to fine tune our white list rules it also helps us to see why $path is reported. For instance:
/run/user/1000/libvirt/libvirt-sock: connect: qemuxml2argvtest: QEMU XML-2-ARGV net-vhostuser-multiq
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virtestmock.c | 39 ++++++++++++++++++++++----------------- 1 file changed, 22 insertions(+), 17 deletions(-)
As noted in patch1 review, not sure how to trigger the above message any more with the fake nwfilter driver connection set. The code appears to be fine to me though, so you have my Reviewed-by: John Ferlan <jferlan@redhat.com> and as noted in patch1 response, I'll leave it up to you in order to determine the need. John
On 08/14/2018 11:54 PM, John Ferlan wrote:
On 07/27/2018 11:24 AM, Michal Privoznik wrote:
As advertised in the previous commit, we need the list of accessed files to also contain action that caused the $path to appear on the list. Not only this enables us to fine tune our white list rules it also helps us to see why $path is reported. For instance:
/run/user/1000/libvirt/libvirt-sock: connect: qemuxml2argvtest: QEMU XML-2-ARGV net-vhostuser-multiq
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virtestmock.c | 39 ++++++++++++++++++++++----------------- 1 file changed, 22 insertions(+), 17 deletions(-)
As noted in patch1 review, not sure how to trigger the above message any more with the fake nwfilter driver connection set. The code appears to be fine to me though, so you have my
Reviewed-by: John Ferlan <jferlan@redhat.com>
and as noted in patch1 response, I'll leave it up to you in order to determine the need.
The point of 2/3 and 3/3 is not to demonstrate the problem that 1/3 is fixing but to detect it should it happen in the future. But you can see these patches in action if you temporarily revert 1/3 and run 'make check-access': libvirt.git/tests $ make check-access | grep connect | grep /run/user /run/user/1000/libvirt: connect: qemuxml2argvtest: QEMU XML-2-ARGV net-vhostuser-multiq Thanks for the review! Michal
On 07/27/2018 05:24 PM, Michal Privoznik wrote:
v3 of:
https://www.redhat.com/archives/libvir-list/2018-July/msg00747.html
diff to v2: - pushed some already ACKed patches - dropped controversial line from 1/3 that turned off error reporting
Michal Prívozník (3): qemuxml2argvtest: Set more fake drivers check-file-access: Allow specifying action virtestmock: Track action
tests/check-file-access.pl | 32 +++++++++++++++++++++++++++----- tests/file_access_whitelist.txt | 15 ++++++++++----- tests/qemuxml2argvtest.c | 4 ++++ tests/virtestmock.c | 39 ++++++++++++++++++++++----------------- 4 files changed, 63 insertions(+), 27 deletions(-)
Ping? Michal
participants (3)
-
Daniel P. Berrangé -
John Ferlan -
Michal Privoznik