6:06 a.m.
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
6:06 a.m.
New subject: [PATCH 1/3] Don't call qsort() over NULL
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(a)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
6:31 a.m.
New subject: [PATCH 1/3] Don't call qsort() over NULL
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(a)redhat.com>
Reviewed-by: Tim Wiederhake <twiederh(a)redhat.com>
6:06 a.m.
New subject: [PATCH 2/3] tests: Don't pass INT_MAX to virFileReadAll()
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(a)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
6:31 a.m.
New subject: [PATCH 2/3] tests: Don't pass INT_MAX to virFileReadAll()
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(a)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;
}
7:14 a.m.
New subject: [PATCH 2/3] tests: Don't pass INT_MAX to virFileReadAll()
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
7:26 a.m.
New subject: [PATCH 2/3] tests: Don't pass INT_MAX to virFileReadAll()
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.
7:30 a.m.
New subject: [PATCH 2/3] tests: Don't pass INT_MAX to virFileReadAll()
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
7:35 a.m.
New subject: [PATCH 2/3] tests: Don't pass INT_MAX to virFileReadAll()
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
9:16 a.m.
New subject: [PATCH 2/3] tests: Don't pass INT_MAX to virFileReadAll()
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
7:31 a.m.
New subject: [PATCH 2/3] tests: Don't pass INT_MAX to virFileReadAll()
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 :|
6:35 a.m.
New subject: [PATCH 2/3] tests: Don't pass INT_MAX to virFileReadAll()
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(a)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.
6:06 a.m.
New subject: [PATCH 3/3] src: Use 1U for bit shifting
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(a)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
6:31 a.m.
New subject: [PATCH 3/3] src: Use 1U for bit shifting
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(a)redhat.com>
Reviewed-by: Tim Wiederhake <twiederh(a)redhat.com>
1257
days inactive
1257
days old
13 comments
5 participants
participants (5)
-
Daniel P. Berrangé -
Michal Privoznik -
Michal Prívozník
-
Peter Krempa -
Tim Wiederhake