[PATCH 0/3] Couple of cleanups
I've noticed these while reviewing Tim's asan patches. Michal Prívozník (3): Don't call qsort() over NULL tests: Don't pass INT_MAX to virFileReadAll() src: Use 1U for bit shifting src/conf/capabilities.c | 6 ++++-- src/conf/domain_capabilities.h | 2 +- src/cpu/cpu_x86.c | 10 +++++----- src/security/security_manager.c | 3 ++- tests/networkxml2firewalltest.c | 2 +- tests/testutils.c | 4 ++-- tools/nss/libvirt_nss.c | 3 ++- 7 files changed, 17 insertions(+), 13 deletions(-) -- 2.31.1
In a few places it may happen that the array we want to sort is still NULL (e.g. because there were no leases found, no paths for secdriver to lock or no cache banks). However, passing NULL to qsort() is undefined and even though glibc plays nicely we shouldn't rely on undefined behaviour. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/capabilities.c | 6 ++++-- src/security/security_manager.c | 3 ++- tools/nss/libvirt_nss.c | 3 ++- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 2f9a1e7d1f..d3b22f7dd0 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -1924,8 +1924,10 @@ virCapabilitiesInitCaches(virCaps *caps) /* Sort the array in order for the tests to be predictable. This way we can * still traverse the directory instead of guessing names (in case there is * 'index1' and 'index3' but no 'index2'). */ - qsort(caps->host.cache.banks, caps->host.cache.nbanks, - sizeof(*caps->host.cache.banks), virCapsHostCacheBankSorter); + if (caps->host.cache.banks) { + qsort(caps->host.cache.banks, caps->host.cache.nbanks, + sizeof(*caps->host.cache.banks), virCapsHostCacheBankSorter); + } if (virCapabilitiesInitResctrlMemory(caps) < 0) goto cleanup; diff --git a/src/security/security_manager.c b/src/security/security_manager.c index d8b84e2861..0af581bc8b 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -1355,7 +1355,8 @@ virSecurityManagerMetadataLock(virSecurityManager *mgr G_GNUC_UNUSED, * paths in the same order and thus no deadlock can occur. * Lastly, it makes searching for duplicate paths below * simpler. */ - qsort(paths, npaths, sizeof(*paths), cmpstringp); + if (paths) + qsort(paths, npaths, sizeof(*paths), cmpstringp); for (i = 0; i < npaths; i++) { const char *p = paths[i]; diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c index b021efc3c9..a6e8e4b12b 100644 --- a/tools/nss/libvirt_nss.c +++ b/tools/nss/libvirt_nss.c @@ -69,7 +69,8 @@ static void sortAddr(leaseAddress *tmpAddress, size_t ntmpAddress) { - qsort(tmpAddress, ntmpAddress, sizeof(*tmpAddress), leaseAddressSorter); + if (tmpAddress) + qsort(tmpAddress, ntmpAddress, sizeof(*tmpAddress), leaseAddressSorter); } -- 2.31.1
On Mon, 2021-06-14 at 13:06 +0200, Michal Privoznik wrote:
In a few places it may happen that the array we want to sort is still NULL (e.g. because there were no leases found, no paths for secdriver to lock or no cache banks). However, passing NULL to qsort() is undefined and even though glibc plays nicely we shouldn't rely on undefined behaviour.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Tim Wiederhake <twiederh@redhat.com>
In a few occasions in tests we pass INT_MAX to virFileReadLimFD(). This is not safe because virFileReadAll() will call virFileReadLimFD() under the hood which takes the limit and adds 1 to it. And since we use signed integer for all of this an overflow will occur. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/networkxml2firewalltest.c | 2 +- tests/testutils.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/networkxml2firewalltest.c b/tests/networkxml2firewalltest.c index 91336a0c55..facbc20a0c 100644 --- a/tests/networkxml2firewalltest.c +++ b/tests/networkxml2firewalltest.c @@ -176,7 +176,7 @@ mymain(void) basefile = g_strdup_printf("%s/networkxml2firewalldata/base.args", abs_srcdir); - if (virFileReadAll(basefile, INT_MAX, &baseargs) < 0) + if (virFileReadAll(basefile, INT_MAX - 1, &baseargs) < 0) return EXIT_FAILURE; DO_TEST("nat-default"); diff --git a/tests/testutils.c b/tests/testutils.c index eb3bd48b6a..4a63c6cc37 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -313,7 +313,7 @@ virTestLoadFileJSON(const char *p, ...) if (!(path = virTestLoadFileGetPath(p, ap))) goto cleanup; - if (virFileReadAll(path, INT_MAX, &jsonstr) < 0) + if (virFileReadAll(path, INT_MAX - 1, &jsonstr) < 0) goto cleanup; if (!(ret = virJSONValueFromString(jsonstr))) @@ -562,7 +562,7 @@ virTestCompareToFileFull(const char *actual, if (virTestLoadFile(filename, &filecontent) < 0 && !virTestGetRegenerate()) return -1; } else { - if (virFileReadAll(filename, INT_MAX, &filecontent) < 0 && !virTestGetRegenerate()) + if (virFileReadAll(filename, INT_MAX - 1, &filecontent) < 0 && !virTestGetRegenerate()) return -1; } -- 2.31.1
On Mon, 2021-06-14 at 13:06 +0200, Michal Privoznik wrote:
In a few occasions in tests we pass INT_MAX to virFileReadLimFD(). This is not safe because virFileReadAll() will call virFileReadLimFD() under the hood which takes the limit and adds 1 to it.
Calling virFileReadAll with "INT_MAX - 1" looks funny. Is it possible to check for "maxlen >= INT_MAX" in virFileReadLimFD instead?
And since we use signed integer for all of this an overflow will occur.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/networkxml2firewalltest.c | 2 +- tests/testutils.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/tests/networkxml2firewalltest.c b/tests/networkxml2firewalltest.c index 91336a0c55..facbc20a0c 100644 --- a/tests/networkxml2firewalltest.c +++ b/tests/networkxml2firewalltest.c @@ -176,7 +176,7 @@ mymain(void) basefile = g_strdup_printf("%s/networkxml2firewalldata/base.args", abs_srcdir); - if (virFileReadAll(basefile, INT_MAX, &baseargs) < 0) + if (virFileReadAll(basefile, INT_MAX - 1, &baseargs) < 0) return EXIT_FAILURE; DO_TEST("nat-default"); diff --git a/tests/testutils.c b/tests/testutils.c index eb3bd48b6a..4a63c6cc37 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -313,7 +313,7 @@ virTestLoadFileJSON(const char *p, ...) if (!(path = virTestLoadFileGetPath(p, ap))) goto cleanup; - if (virFileReadAll(path, INT_MAX, &jsonstr) < 0) + if (virFileReadAll(path, INT_MAX - 1, &jsonstr) < 0) goto cleanup; if (!(ret = virJSONValueFromString(jsonstr))) @@ -562,7 +562,7 @@ virTestCompareToFileFull(const char *actual, if (virTestLoadFile(filename, &filecontent) < 0 && !virTestGetRegenerate()) return -1; } else { - if (virFileReadAll(filename, INT_MAX, &filecontent) < 0 && !virTestGetRegenerate()) + if (virFileReadAll(filename, INT_MAX - 1, &filecontent) < 0 && !virTestGetRegenerate()) return -1; }
On 6/14/21 1:31 PM, Tim Wiederhake wrote:
On Mon, 2021-06-14 at 13:06 +0200, Michal Privoznik wrote:
In a few occasions in tests we pass INT_MAX to virFileReadLimFD(). This is not safe because virFileReadAll() will call virFileReadLimFD() under the hood which takes the limit and adds 1 to it.
Calling virFileReadAll with "INT_MAX - 1" looks funny. Is it possible to check for "maxlen >= INT_MAX" in virFileReadLimFD instead?
Actually, I don't understand why we need to add 1 in the first place. I'll push the other two patches and send v2 for this that removes the +1. Michal
On Mon, Jun 14, 2021 at 14:14:47 +0200, Michal Prívozník wrote:
On 6/14/21 1:31 PM, Tim Wiederhake wrote:
On Mon, 2021-06-14 at 13:06 +0200, Michal Privoznik wrote:
In a few occasions in tests we pass INT_MAX to virFileReadLimFD(). This is not safe because virFileReadAll() will call virFileReadLimFD() under the hood which takes the limit and adds 1 to it.
Calling virFileReadAll with "INT_MAX - 1" looks funny. Is it possible to check for "maxlen >= INT_MAX" in virFileReadLimFD instead?
Actually, I don't understand why we need to add 1 in the first place. I'll push the other two patches and send v2 for this that removes the +1.
It's so that it guarantees that a file of 'maxlen' length is read completely and the terminating '\0' is in the resulting string. Removing the '+ 1' would change this kind of semantics, which may require audit of all callers.
On 6/14/21 2:26 PM, Peter Krempa wrote:
On Mon, Jun 14, 2021 at 14:14:47 +0200, Michal Prívozník wrote:
On 6/14/21 1:31 PM, Tim Wiederhake wrote:
On Mon, 2021-06-14 at 13:06 +0200, Michal Privoznik wrote:
In a few occasions in tests we pass INT_MAX to virFileReadLimFD(). This is not safe because virFileReadAll() will call virFileReadLimFD() under the hood which takes the limit and adds 1 to it.
Calling virFileReadAll with "INT_MAX - 1" looks funny. Is it possible to check for "maxlen >= INT_MAX" in virFileReadLimFD instead?
Actually, I don't understand why we need to add 1 in the first place. I'll push the other two patches and send v2 for this that removes the +1.
It's so that it guarantees that a file of 'maxlen' length is read completely and the terminating '\0' is in the resulting string.
Removing the '+ 1' would change this kind of semantics, which may require audit of all callers.
I'm not sure that's correct behaviour. I mean, if I specify that the limit should be X, then at the most X bytes should be read, not X+1. Also, virFileReadLimFD() uses saferead_lim() which upon successful return makes sure the returned string is properly terminated. Michal
On Mon, Jun 14, 2021 at 14:30:26 +0200, Michal Prívozník wrote:
On 6/14/21 2:26 PM, Peter Krempa wrote:
On Mon, Jun 14, 2021 at 14:14:47 +0200, Michal Prívozník wrote:
On 6/14/21 1:31 PM, Tim Wiederhake wrote:
On Mon, 2021-06-14 at 13:06 +0200, Michal Privoznik wrote:
In a few occasions in tests we pass INT_MAX to virFileReadLimFD(). This is not safe because virFileReadAll() will call virFileReadLimFD() under the hood which takes the limit and adds 1 to it.
Calling virFileReadAll with "INT_MAX - 1" looks funny. Is it possible to check for "maxlen >= INT_MAX" in virFileReadLimFD instead?
Actually, I don't understand why we need to add 1 in the first place. I'll push the other two patches and send v2 for this that removes the +1.
It's so that it guarantees that a file of 'maxlen' length is read completely and the terminating '\0' is in the resulting string.
Removing the '+ 1' would change this kind of semantics, which may require audit of all callers.
I'm not sure that's correct behaviour. I mean, if I specify that the limit should be X, then at the most X bytes should be read, not X+1.
Also, virFileReadLimFD() uses saferead_lim() which upon successful return makes sure the returned string is properly terminated.
saferead_lim indeed terminates the string properly. virFileReadLimFD uses the '+ 1' to see whether there are exactly "maxlen" chars in the file or potentially more than that. That's why it actually reads 1 more than the requested maximum. Whether that makes sense is obviously questionable and will require audit of all callers
On 6/14/21 2:35 PM, Peter Krempa wrote:
On Mon, Jun 14, 2021 at 14:30:26 +0200, Michal Prívozník wrote:
On 6/14/21 2:26 PM, Peter Krempa wrote:
On Mon, Jun 14, 2021 at 14:14:47 +0200, Michal Prívozník wrote:
On 6/14/21 1:31 PM, Tim Wiederhake wrote:
On Mon, 2021-06-14 at 13:06 +0200, Michal Privoznik wrote:
In a few occasions in tests we pass INT_MAX to virFileReadLimFD(). This is not safe because virFileReadAll() will call virFileReadLimFD() under the hood which takes the limit and adds 1 to it.
Calling virFileReadAll with "INT_MAX - 1" looks funny. Is it possible to check for "maxlen >= INT_MAX" in virFileReadLimFD instead?
Actually, I don't understand why we need to add 1 in the first place. I'll push the other two patches and send v2 for this that removes the +1.
It's so that it guarantees that a file of 'maxlen' length is read completely and the terminating '\0' is in the resulting string.
Removing the '+ 1' would change this kind of semantics, which may require audit of all callers.
I'm not sure that's correct behaviour. I mean, if I specify that the limit should be X, then at the most X bytes should be read, not X+1.
Also, virFileReadLimFD() uses saferead_lim() which upon successful return makes sure the returned string is properly terminated.
saferead_lim indeed terminates the string properly. virFileReadLimFD uses the '+ 1' to see whether there are exactly "maxlen" chars in the file or potentially more than that. That's why it actually reads 1 more than the requested maximum.
Whether that makes sense is obviously questionable and will require audit of all callers
OTOH, now that I made the change locally, there's no difference between virFileReadHeaderFD() and virFileReadLimFD(). IOW, if there is a caller that wants to read a file fully it won't have a way to do that. Although, I've looked at all calls of virFileReadLimFD() (and all transitive places like virFileReadAll() and virFileReadAllQuiet()) and all pass arbitrary limits but never retry with bigger limit on error. Special casing INT_MAX in virFileReadLimFD() looks weird to me. So I guess, in the end I'll put this into "known bug" section and carry on with my life. Unless we want to do something as wild as: s = saferead_lim(fd, (size_t)maxlen + 1, &len); Michal
On Mon, Jun 14, 2021 at 02:26:48PM +0200, Peter Krempa wrote:
On Mon, Jun 14, 2021 at 14:14:47 +0200, Michal Prívozník wrote:
On 6/14/21 1:31 PM, Tim Wiederhake wrote:
On Mon, 2021-06-14 at 13:06 +0200, Michal Privoznik wrote:
In a few occasions in tests we pass INT_MAX to virFileReadLimFD(). This is not safe because virFileReadAll() will call virFileReadLimFD() under the hood which takes the limit and adds 1 to it.
Calling virFileReadAll with "INT_MAX - 1" looks funny. Is it possible to check for "maxlen >= INT_MAX" in virFileReadLimFD instead?
Actually, I don't understand why we need to add 1 in the first place. I'll push the other two patches and send v2 for this that removes the +1.
It's so that it guarantees that a file of 'maxlen' length is read completely and the terminating '\0' is in the resulting string.
Removing the '+ 1' would change this kind of semantics, which may require audit of all callers.
Isnt it just a matter of semantics of 'maxlen'. Your description is saying the semantics for 'maxlen' are the total length of file plus a possible trailing null. Can we just define 'maxlen' to mean the total length of the file, not including an extra trailing null ? ie so that 'maxlen' is essentially equal to strlen(buf) in the case where the file has no embedded nuls 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 Mon, Jun 14, 2021 at 13:06:13 +0200, Michal Privoznik wrote:
In a few occasions in tests we pass INT_MAX to virFileReadLimFD(). This is not safe because virFileReadAll() will call virFileReadLimFD() under the hood which takes the limit and adds 1 to it. And since we use signed integer for all of this an overflow will occur.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/networkxml2firewalltest.c | 2 +- tests/testutils.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/tests/networkxml2firewalltest.c b/tests/networkxml2firewalltest.c index 91336a0c55..facbc20a0c 100644 --- a/tests/networkxml2firewalltest.c +++ b/tests/networkxml2firewalltest.c @@ -176,7 +176,7 @@ mymain(void)
basefile = g_strdup_printf("%s/networkxml2firewalldata/base.args", abs_srcdir);
- if (virFileReadAll(basefile, INT_MAX, &baseargs) < 0) + if (virFileReadAll(basefile, INT_MAX - 1, &baseargs) < 0)
While you are fixing all instances of this problem this won't fix any further mistakes that can happen. At the very least you should document this quirk in the function header.
In a few places we take 1 and shift it left repeatedly. So much that it won't longer fit into signed integer. The problem is that this is undefined behaviour. Switching to 1U makes us stay within boundaries. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_capabilities.h | 2 +- src/cpu/cpu_x86.c | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h index 69e90893cc..b6433b20c9 100644 --- a/src/conf/domain_capabilities.h +++ b/src/conf/domain_capabilities.h @@ -231,7 +231,7 @@ virDomainCapsCPUModelsGet(virDomainCapsCPUModels *cpuModels, const char *name); #define VIR_DOMAIN_CAPS_ENUM_IS_SET(capsEnum, value) \ - ((capsEnum).values & (1 << value)) + ((capsEnum).values & (1U << value)) #define VIR_DOMAIN_CAPS_ENUM_SET(capsEnum, ...) \ do { \ diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index a4599499d0..a4792c21da 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -2575,12 +2575,12 @@ cpuidSetLeafD(virCPUData *data, sub1 = *cpuid; for (sub = 2; sub < 64; sub++) { if (sub < 32 && - !(sub0.eax & (1 << sub)) && - !(sub1.ecx & (1 << sub))) + !(sub0.eax & (1U << sub)) && + !(sub1.ecx & (1U << sub))) continue; if (sub >= 32 && - !(sub0.edx & (1 << (sub - 32))) && - !(sub1.edx & (1 << (sub - 32)))) + !(sub0.edx & (1U << (sub - 32))) && + !(sub1.edx & (1U << (sub - 32)))) continue; cpuid->ecx_in = sub; @@ -2614,7 +2614,7 @@ cpuidSetLeafResID(virCPUData *data, return -1; for (sub = 1; sub < 32; sub++) { - if (!(res & (1 << sub))) + if (!(res & (1U << sub))) continue; cpuid->ecx_in = sub; cpuidCall(cpuid); -- 2.31.1
On Mon, 2021-06-14 at 13:06 +0200, Michal Privoznik wrote:
In a few places we take 1 and shift it left repeatedly. So much that it won't longer fit into signed integer. The problem is that this is undefined behaviour. Switching to 1U makes us stay within boundaries.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Tim Wiederhake <twiederh@redhat.com>
participants (5)
-
Daniel P. Berrangé -
Michal Privoznik -
Michal Prívozník
-
Peter Krempa -
Tim Wiederhake