7:34 a.m.
Ján Tomko (7):
nwfilter: fix NULL pointer check in virNWFilterSnoopReqNew
conf: prevent crash with no uuid in cephx auth secret
cgroup: fix impossible overrun in virCgroupAddTaskController
libssh2_session: support DSS keys as well
virsh: fix error messages in iface-bridge
conf: check the return value of virXPathNodeSet
conf: snapshot: check return value of virDomainSnapshotObjListNum
src/conf/domain_conf.c | 8 ++++++--
src/conf/snapshot_conf.c | 2 +-
src/conf/storage_conf.c | 12 ++++++------
src/nwfilter/nwfilter_dhcpsnoop.c | 4 ++--
src/rpc/virnetsshsession.c | 1 +
src/util/cgroup.c | 2 +-
tools/virsh-interface.c | 4 ++--
7 files changed, 19 insertions(+), 14 deletions(-)
--
1.7.8.6
7:34 a.m.
New subject: [libvirt] [PATCH 1/7] nwfilter: fix NULL pointer check in virNWFilterSnoopReqNew
This can't lead to a crash since virNWFilterSnoopReqNew is only called
with a static array as the argument, but if we check for NULL we should
do it right.
---
src/nwfilter/nwfilter_dhcpsnoop.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c
index 807fd28..7703efd 100644
--- a/src/nwfilter/nwfilter_dhcpsnoop.c
+++ b/src/nwfilter/nwfilter_dhcpsnoop.c
@@ -573,12 +573,12 @@ virNWFilterSnoopReqNew(const char *ifkey)
{
virNWFilterSnoopReqPtr req;
- if (ifkey == NULL || strlen(ifkey) != VIR_IFKEY_LEN - 1) {
+ if (ifkey == NULL || (ifkey && strlen(ifkey) != VIR_IFKEY_LEN - 1)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("virNWFilterSnoopReqNew called with invalid "
"key \"%s\" (%zu)"),
ifkey ? ifkey : "",
- strlen(ifkey));
+ ifkey ? strlen(ifkey) : 0);
return NULL;
}
--
1.7.8.6
8:33 a.m.
New subject: [libvirt] [PATCH 1/7] nwfilter: fix NULL pointer check in virNWFilterSnoopReqNew
On 2012年11月28日 21:34, Ján Tomko wrote:
This can't lead to a crash since virNWFilterSnoopReqNew is only
called
with a static array as the argument, but if we check for NULL we should
do it right.
---
src/nwfilter/nwfilter_dhcpsnoop.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c
index 807fd28..7703efd 100644
--- a/src/nwfilter/nwfilter_dhcpsnoop.c
+++ b/src/nwfilter/nwfilter_dhcpsnoop.c
@@ -573,12 +573,12 @@ virNWFilterSnoopReqNew(const char *ifkey)
{
virNWFilterSnoopReqPtr req;
- if (ifkey == NULL || strlen(ifkey) != VIR_IFKEY_LEN - 1) {
+ if (ifkey == NULL || (ifkey&& strlen(ifkey) != VIR_IFKEY_LEN - 1)) {
Good catch, but personally I'd like use brackets for "strlen" expression
too.
virReportError(VIR_ERR_INTERNAL_ERROR,
_("virNWFilterSnoopReqNew called with invalid "
"key \"%s\" (%zu)"),
ifkey ? ifkey : "",
- strlen(ifkey));
+ ifkey ? strlen(ifkey) : 0);
return NULL;
}
ACK.
10:11 a.m.
New subject: [libvirt] [PATCH 1/7] nwfilter: fix NULL pointer check in virNWFilterSnoopReqNew
On 2012年11月28日 21:34, Ján Tomko wrote:
> This can't lead to a crash since virNWFilterSnoopReqNew is only
> called
> with a static array as the argument, but if we check for NULL we
> should
> do it right.
>
> - if (ifkey == NULL || strlen(ifkey) != VIR_IFKEY_LEN - 1) {
> + if (ifkey == NULL || (ifkey&& strlen(ifkey) != VIR_IFKEY_LEN
> - 1)) {
Good catch, but personally I'd like use brackets for "strlen"
expression
too.
This hunk is pointless. You cannot get to the right side of || unless
ifkey is non-NULL on the left side. We should revert this hunk.
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("virNWFilterSnoopReqNew called with
> invalid "
> "key \"%s\" (%zu)"),
> ifkey ? ifkey : "",
> - strlen(ifkey));
> + ifkey ? strlen(ifkey) : 0);
This is the real fix, and should be kept.
8:04 p.m.
New subject: [libvirt] [PATCH 1/7] nwfilter: fix NULL pointer check in virNWFilterSnoopReqNew
On 2012年11月29日 00:11, Eric Blake wrote:
> On 2012年11月28日 21:34, Ján Tomko wrote:
>> This can't lead to a crash since virNWFilterSnoopReqNew is only
>> called
>> with a static array as the argument, but if we check for NULL we
>> should
>> do it right.
>>
>> - if (ifkey == NULL || strlen(ifkey) != VIR_IFKEY_LEN - 1) {
>> + if (ifkey == NULL || (ifkey&& strlen(ifkey) != VIR_IFKEY_LEN
>> - 1)) {
>
> Good catch, but personally I'd like use brackets for "strlen"
> expression
> too.
This hunk is pointless. You cannot get to the right side of || unless
ifkey is non-NULL on the left side. We should revert this hunk.
ooha, okay, thanks for fixing that.
Osier
7:34 a.m.
New subject: [libvirt] [PATCH 2/7] conf: prevent crash with no uuid in cephx auth secret
Also remove the pointles check for NULL in auth.cephx.secret.uuid,
since this is a static array.
---
src/conf/storage_conf.c | 8 +++-----
1 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 1c9934c..5a2cf1b 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -452,7 +452,7 @@ virStoragePoolDefParseAuthCephx(xmlXPathContextPtr ctxt,
uuid = virXPathString("string(./auth/secret/@uuid)", ctxt);
auth->secret.usage = virXPathString("string(./auth/secret/@usage)",
ctxt);
- if (uuid == NULL && auth->secret.usage == NULL) {
+ if (uuid == NULL || auth->secret.usage == NULL) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("missing auth secret uuid or usage attribute"));
return -1;
@@ -976,10 +976,8 @@ virStoragePoolSourceFormat(virBufferPtr buf,
src->auth.cephx.username);
virBufferAsprintf(buf," %s", "<secret");
- if (src->auth.cephx.secret.uuid != NULL) {
- virUUIDFormat(src->auth.cephx.secret.uuid, uuid);
- virBufferAsprintf(buf," uuid='%s'", uuid);
- }
+ virUUIDFormat(src->auth.cephx.secret.uuid, uuid);
+ virBufferAsprintf(buf," uuid='%s'", uuid);
if (src->auth.cephx.secret.usage != NULL) {
virBufferAsprintf(buf," usage='%s'",
src->auth.cephx.secret.usage);
--
1.7.8.6
8:31 a.m.
New subject: [libvirt] [PATCH 2/7] conf: prevent crash with no uuid in cephx auth secret
On 2012年11月28日 21:34, Ján Tomko wrote:
Also remove the pointles check for NULL in auth.cephx.secret.uuid,
since this is a static array.
It's nice if there is log of coverity.
---
src/conf/storage_conf.c | 8 +++-----
1 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 1c9934c..5a2cf1b 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -452,7 +452,7 @@ virStoragePoolDefParseAuthCephx(xmlXPathContextPtr ctxt,
uuid = virXPathString("string(./auth/secret/@uuid)", ctxt);
auth->secret.usage = virXPathString("string(./auth/secret/@usage)",
ctxt);
- if (uuid == NULL&& auth->secret.usage == NULL) {
+ if (uuid == NULL || auth->secret.usage == NULL) {
This change forces both of "uuid" and "usage" to be specified.
But from the RNG schema:
<define name='sourceinfoauthsecret'>
<element name='secret'>
<choice>
<attribute name='uuid'>
<text/>
</attribute>
<attribute name='usage'>
<text/>
</attribute>
</choice>
</element>
</define>
Means that it allows only one of them specified.
Hm, from the schema, it should error out if both of them are
specified too. So either there is problem of either the schema
or the codes.
I think we have to figure out if the schema is correct first.
virReportError(VIR_ERR_XML_ERROR, "%s",
_("missing auth secret uuid or usage attribute"));
return -1;
@@ -976,10 +976,8 @@ virStoragePoolSourceFormat(virBufferPtr buf,
src->auth.cephx.username);
virBufferAsprintf(buf," %s", "<secret");
- if (src->auth.cephx.secret.uuid != NULL) {
- virUUIDFormat(src->auth.cephx.secret.uuid, uuid);
- virBufferAsprintf(buf," uuid='%s'", uuid);
- }
+ virUUIDFormat(src->auth.cephx.secret.uuid, uuid);
+ virBufferAsprintf(buf," uuid='%s'", uuid);
This is fine though.
if (src->auth.cephx.secret.usage != NULL) {
virBufferAsprintf(buf," usage='%s'",
src->auth.cephx.secret.usage);
Osier
10:21 a.m.
New subject: [libvirt] [PATCH 2/7] conf: prevent crash with no uuid in cephx auth secret
On 11/28/12 15:31, Osier Yang wrote:
On 2012年11月28日 21:34, Ján Tomko wrote:
> Also remove the pointles check for NULL in auth.cephx.secret.uuid,
> since this is a static array.
It's nice if there is log of coverity.
Error: FORWARD_NULL (CWE-476):
libvirt-0.10.2/src/conf/storage_conf.c:447: cond_false: Condition
"auth->username == NULL", taking false branch
libvirt-0.10.2/src/conf/storage_conf.c:451: if_end: End of if statement
libvirt-0.10.2/src/conf/storage_conf.c:455: cond_true: Condition "uuid
== NULL", taking true branch
libvirt-0.10.2/src/conf/storage_conf.c:455: var_compare_op: Comparing
"uuid" to null implies that "uuid" might be null.
libvirt-0.10.2/src/conf/storage_conf.c:455: cond_false: Condition
"auth->secret.usage == NULL", taking false branch
libvirt-0.10.2/src/conf/storage_conf.c:459: if_end: End of if statement
libvirt-0.10.2/src/conf/storage_conf.c:461: var_deref_model: Passing
null pointer "uuid" to function "virUUIDParse(char const *, unsigned
char *)", which dereferences it. (The dereference is assumed on the
basis of the 'nonnull' parameter attribute.)
Error: NO_EFFECT (CWE-398):
libvirt-0.10.2/src/conf/storage_conf.c:979: array_null: Comparing an
array to null is not useful: "src->auth.cephx.secret.uuid != NULL".
> ...
This change forces both of "uuid" and "usage" to be specified.
But from the RNG schema:
<define name='sourceinfoauthsecret'>
<element name='secret'>
<choice>
<attribute name='uuid'>
<text/>
</attribute>
<attribute name='usage'>
<text/>
</attribute>
</choice>
</element>
</define>
Means that it allows only one of them specified.
Hm, from the schema, it should error out if both of them are
specified too. So either there is problem of either the schema
or the codes.
I think we have to figure out if the schema is correct first.
Looking at the code in storage_backend_rbd.c it looks like if both are
specified, only usage is taken into account:
if (pool->def->source.auth.cephx.secret.uuid != NULL) {
virUUIDFormat(pool->def->source.auth.cephx.secret.uuid, secretUuid);
VIR_DEBUG("Looking up secret by UUID: %s", secretUuid);
secret = virSecretLookupByUUIDString(conn, secretUuid);
}
if (pool->def->source.auth.cephx.secret.usage != NULL) {
VIR_DEBUG("Looking up secret by usage: %s",
pool->def->source.auth.cephx.secret.usage);
secret = virSecretLookupByUsage(conn, VIR_SECRET_USAGE_TYPE_CEPH,
pool->def->source.auth.cephx.secret.usage);
}
I'll send another version shortly.
Jan
10:36 a.m.
New subject: [libvirt] [PATCH v2] conf: prevent crash with no uuid in cephx auth secret
Also remove the pointless check for NULL in auth.cephx.secret.uuid,
since this is a static array.
---
src/conf/storage_conf.c | 8 +++-----
1 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 3fdc5b6..99c2e52 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -458,7 +458,7 @@ virStoragePoolDefParseAuthCephx(xmlXPathContextPtr ctxt,
return -1;
}
- if (virUUIDParse(uuid, auth->secret.uuid) < 0) {
+ if (uuid && virUUIDParse(uuid, auth->secret.uuid) < 0) {
virReportError(VIR_ERR_XML_ERROR,
"%s", _("invalid auth secret uuid"));
return -1;
@@ -979,10 +979,8 @@ virStoragePoolSourceFormat(virBufferPtr buf,
src->auth.cephx.username);
virBufferAsprintf(buf," %s", "<secret");
- if (src->auth.cephx.secret.uuid != NULL) {
- virUUIDFormat(src->auth.cephx.secret.uuid, uuid);
- virBufferAsprintf(buf," uuid='%s'", uuid);
- }
+ virUUIDFormat(src->auth.cephx.secret.uuid, uuid);
+ virBufferAsprintf(buf," uuid='%s'", uuid);
if (src->auth.cephx.secret.usage != NULL) {
virBufferAsprintf(buf," usage='%s'",
src->auth.cephx.secret.usage);
--
1.7.8.6
5:20 a.m.
New subject: [libvirt] [PATCH v2] conf: prevent crash with no uuid in cephx auth secret
On 11/29/12 17:36, Ján Tomko wrote:
Also remove the pointless check for NULL in auth.cephx.secret.uuid,
since this is a static array.
---
src/conf/storage_conf.c | 8 +++-----
1 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 3fdc5b6..99c2e52 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -458,7 +458,7 @@ virStoragePoolDefParseAuthCephx(xmlXPathContextPtr ctxt,
return -1;
}
- if (virUUIDParse(uuid, auth->secret.uuid) < 0) {
+ if (uuid && virUUIDParse(uuid, auth->secret.uuid) < 0) {
Given the XML schema that was discussed in the previous version of this
patch, this hunk is OK, but it doesn't mark the secret that it doesn't
have an UUID. The old check that was present wasn't effective enough.
With this patch ceph secrets that have just the usage argument will have
an UUID full of zeroes.
virReportError(VIR_ERR_XML_ERROR,
"%s", _("invalid auth secret uuid"));
return -1;
@@ -979,10 +979,8 @@ virStoragePoolSourceFormat(virBufferPtr buf,
src->auth.cephx.username);
virBufferAsprintf(buf," %s", "<secret");
- if (src->auth.cephx.secret.uuid != NULL) {
- virUUIDFormat(src->auth.cephx.secret.uuid, uuid);
- virBufferAsprintf(buf," uuid='%s'", uuid);
- }
+ virUUIDFormat(src->auth.cephx.secret.uuid, uuid);
+ virBufferAsprintf(buf," uuid='%s'", uuid);
And it will be formated as such here.
if (src->auth.cephx.secret.usage != NULL) {
virBufferAsprintf(buf," usage='%s'",
src->auth.cephx.secret.usage);
And in virStorageBackendRBDOpenRADOSConn() in
"src/storage/storage_backend_rbd.c":
if (pool->def->source.auth.cephx.secret.uuid != NULL) {
virUUIDFormat(pool->def->source.auth.cephx.secret.uuid, secretUuid);
VIR_DEBUG("Looking up secret by UUID: %s", secretUuid);
secret = virSecretLookupByUUIDString(conn, secretUuid);
}
if (pool->def->source.auth.cephx.secret.usage != NULL) {
VIR_DEBUG("Looking up secret by usage: %s",
pool->def->source.auth.cephx.secret.usage);
secret = virSecretLookupByUsage(conn, VIR_SECRET_USAGE_TYPE_CEPH,
pool->def->source.auth.cephx.secret.usage);
}
If such a secret exists, it will be found by the UUID string at first
and then overwritten when looked up by usage, this would be OK, if we
wouldn't have to free the "secret" value afterwards. This causes a memleak.
As a fix, the options (if the schema actually is ok and UUID can be left
out) that come into my mind are:
1) add a new (bool) field to the secret value holding if UUID was provided
2) converting UUID to a pointer and marking the missing value with NULL
as it was before.
Option 1 is probably better as you don't have to keep in mind to free
the UUID array afterwards.
Peter
6:35 a.m.
New subject: [libvirt] [PATCHv3] conf: prevent crash with no uuid in cephx auth secret
Fix the null pointer access when UUID is not specified.
Introduce a bool 'uuidUsable' to virStoragePoolAuthCephx that indicates
if uuid was specified or not and use it instead of the pointless
comparison of the static UUID array to NULL.
Add an error message if both uuid and usage are specified.
Fixes:
Error: FORWARD_NULL (CWE-476):
libvirt-0.10.2/src/conf/storage_conf.c:461: var_deref_model: Passing
null pointer "uuid" to function "virUUIDParse(char const *, unsigned
char *)", which dereferences it. (The dereference is assumed on the
basis of the 'nonnull' parameter attribute.)
Error: NO_EFFECT (CWE-398):
libvirt-0.10.2/src/conf/storage_conf.c:979: array_null: Comparing an
array to null is not useful: "src->auth.cephx.secret.uuid != NULL".
---
src/conf/storage_conf.c | 20 +++++++++++++++-----
src/conf/storage_conf.h | 1 +
src/storage/storage_backend_rbd.c | 6 ++----
3 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 3fdc5b6..a6c6ce7 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -458,10 +458,20 @@ virStoragePoolDefParseAuthCephx(xmlXPathContextPtr ctxt,
return -1;
}
- if (virUUIDParse(uuid, auth->secret.uuid) < 0) {
- virReportError(VIR_ERR_XML_ERROR,
- "%s", _("invalid auth secret uuid"));
- return -1;
+ if (uuid != NULL) {
+ if (auth->secret.usage != NULL) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("either auth secret uuid or usage expected"));
+ return -1;
+ }
+ if (virUUIDParse(uuid, auth->secret.uuid) < 0) {
+ virReportError(VIR_ERR_XML_ERROR,
+ "%s", _("invalid auth secret uuid"));
+ return -1;
+ }
+ auth->secret.uuidUsable = true;
+ } else {
+ auth->secret.uuidUsable = false;
}
return 0;
@@ -979,7 +989,7 @@ virStoragePoolSourceFormat(virBufferPtr buf,
src->auth.cephx.username);
virBufferAsprintf(buf," %s", "<secret");
- if (src->auth.cephx.secret.uuid != NULL) {
+ if (src->auth.cephx.secret.uuidUsable) {
virUUIDFormat(src->auth.cephx.secret.uuid, uuid);
virBufferAsprintf(buf," uuid='%s'", uuid);
}
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h
index d509b13..743b768 100644
--- a/src/conf/storage_conf.h
+++ b/src/conf/storage_conf.h
@@ -169,6 +169,7 @@ struct _virStoragePoolAuthCephx {
struct {
unsigned char uuid[VIR_UUID_BUFLEN];
char *usage;
+ bool uuidUsable;
} secret;
};
diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c
index 0c9bdcc..bc61cf7 100644
--- a/src/storage/storage_backend_rbd.c
+++ b/src/storage/storage_backend_rbd.c
@@ -70,13 +70,11 @@ static int
virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr *ptr,
goto cleanup;
}
- if (pool->def->source.auth.cephx.secret.uuid != NULL) {
+ if (pool->def->source.auth.cephx.secret.uuidUsable) {
virUUIDFormat(pool->def->source.auth.cephx.secret.uuid, secretUuid);
VIR_DEBUG("Looking up secret by UUID: %s", secretUuid);
secret = virSecretLookupByUUIDString(conn, secretUuid);
- }
-
- if (pool->def->source.auth.cephx.secret.usage != NULL) {
+ } else if (pool->def->source.auth.cephx.secret.usage != NULL) {
VIR_DEBUG("Looking up secret by usage: %s",
pool->def->source.auth.cephx.secret.usage);
secret = virSecretLookupByUsage(conn, VIR_SECRET_USAGE_TYPE_CEPH,
--
1.7.8.6
8:04 a.m.
New subject: [libvirt] [PATCHv3] conf: prevent crash with no uuid in cephx auth secret
On 12/03/12 13:35, Ján Tomko wrote:
Fix the null pointer access when UUID is not specified.
Introduce a bool 'uuidUsable' to virStoragePoolAuthCephx that indicates
if uuid was specified or not and use it instead of the pointless
comparison of the static UUID array to NULL.
Add an error message if both uuid and usage are specified.
Fixes:
Error: FORWARD_NULL (CWE-476):
libvirt-0.10.2/src/conf/storage_conf.c:461: var_deref_model: Passing
null pointer "uuid" to function "virUUIDParse(char const *, unsigned
char *)", which dereferences it. (The dereference is assumed on the
basis of the 'nonnull' parameter attribute.)
Error: NO_EFFECT (CWE-398):
libvirt-0.10.2/src/conf/storage_conf.c:979: array_null: Comparing an
array to null is not useful: "src->auth.cephx.secret.uuid != NULL".
---
src/conf/storage_conf.c | 20 +++++++++++++++-----
src/conf/storage_conf.h | 1 +
src/storage/storage_backend_rbd.c | 6 ++----
3 files changed, 18 insertions(+), 9 deletions(-)
Now it looks OK to me. ACK.
Peter
8:15 a.m.
New subject: [libvirt] [PATCHv3] conf: prevent crash with no uuid in cephx auth secret
On 12/03/12 15:04, Peter Krempa wrote:
On 12/03/12 13:35, Ján Tomko wrote:
> Fix the null pointer access when UUID is not specified.
> Introduce a bool 'uuidUsable' to virStoragePoolAuthCephx that indicates
> if uuid was specified or not and use it instead of the pointless
> comparison of the static UUID array to NULL.
> Add an error message if both uuid and usage are specified.
>
> Fixes:
> Error: FORWARD_NULL (CWE-476):
> libvirt-0.10.2/src/conf/storage_conf.c:461: var_deref_model: Passing
> null pointer "uuid" to function "virUUIDParse(char const *,
unsigned
> char *)", which dereferences it. (The dereference is assumed on the
> basis of the 'nonnull' parameter attribute.)
> Error: NO_EFFECT (CWE-398):
> libvirt-0.10.2/src/conf/storage_conf.c:979: array_null: Comparing an
> array to null is not useful: "src->auth.cephx.secret.uuid !=
NULL".
> ---
> src/conf/storage_conf.c | 20 +++++++++++++++-----
> src/conf/storage_conf.h | 1 +
> src/storage/storage_backend_rbd.c | 6 ++----
> 3 files changed, 18 insertions(+), 9 deletions(-)
>
Now it looks OK to me. ACK.
Peter
And pushed.
7:34 a.m.
New subject: [libvirt] [PATCH 3/7] cgroup: fix impossible overrun in virCgroupAddTaskController
The size of the controllers array is VIR_CGROUP_CONTROLLER_LAST, however
we only call it with values less than VIR_CGROUP_CONTROLLER_LAST.
---
src/util/cgroup.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/util/cgroup.c b/src/util/cgroup.c
index 9e78314..490f1de 100644
--- a/src/util/cgroup.c
+++ b/src/util/cgroup.c
@@ -814,7 +814,7 @@ int virCgroupAddTask(virCgroupPtr group, pid_t pid)
*/
int virCgroupAddTaskController(virCgroupPtr group, pid_t pid, int controller)
{
- if (controller < 0 || controller > VIR_CGROUP_CONTROLLER_LAST)
+ if (controller < 0 || controller >= VIR_CGROUP_CONTROLLER_LAST)
return -EINVAL;
if (!group->controllers[controller].mountPoint)
--
1.7.8.6
8:37 a.m.
New subject: [libvirt] [PATCH 3/7] cgroup: fix impossible overrun in virCgroupAddTaskController
On 2012年11月28日 21:34, Ján Tomko wrote:
The size of the controllers array is VIR_CGROUP_CONTROLLER_LAST,
however
we only call it with values less than VIR_CGROUP_CONTROLLER_LAST.
---
src/util/cgroup.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/util/cgroup.c b/src/util/cgroup.c
index 9e78314..490f1de 100644
--- a/src/util/cgroup.c
+++ b/src/util/cgroup.c
@@ -814,7 +814,7 @@ int virCgroupAddTask(virCgroupPtr group, pid_t pid)
*/
int virCgroupAddTaskController(virCgroupPtr group, pid_t pid, int controller)
{
- if (controller< 0 || controller> VIR_CGROUP_CONTROLLER_LAST)
+ if (controller< 0 || controller>= VIR_CGROUP_CONTROLLER_LAST)
return -EINVAL;
if (!group->controllers[controller].mountPoint)
ACK.
7:34 a.m.
New subject: [libvirt] [PATCH 4/7] libssh2_session: support DSS keys as well
Missing break in the switch.
---
src/rpc/virnetsshsession.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/rpc/virnetsshsession.c b/src/rpc/virnetsshsession.c
index 13a0368..286cc4d 100644
--- a/src/rpc/virnetsshsession.c
+++ b/src/rpc/virnetsshsession.c
@@ -412,6 +412,7 @@ virNetSSHCheckHostKey(virNetSSHSessionPtr sess)
break;
case LIBSSH2_HOSTKEY_TYPE_DSS:
keyType = LIBSSH2_KNOWNHOST_KEY_SSHDSS;
+ break;
case LIBSSH2_HOSTKEY_TYPE_UNKNOWN:
default:
--
1.7.8.6
8:38 a.m.
New subject: [libvirt] [PATCH 4/7] libssh2_session: support DSS keys as well
On 2012年11月28日 21:34, Ján Tomko wrote:
Missing break in the switch.
---
src/rpc/virnetsshsession.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/rpc/virnetsshsession.c b/src/rpc/virnetsshsession.c
index 13a0368..286cc4d 100644
--- a/src/rpc/virnetsshsession.c
+++ b/src/rpc/virnetsshsession.c
@@ -412,6 +412,7 @@ virNetSSHCheckHostKey(virNetSSHSessionPtr sess)
break;
case LIBSSH2_HOSTKEY_TYPE_DSS:
keyType = LIBSSH2_KNOWNHOST_KEY_SSHDSS;
+ break;
case LIBSSH2_HOSTKEY_TYPE_UNKNOWN:
default:
ACK
7:34 a.m.
New subject: [libvirt] [PATCH 5/7] virsh: fix error messages in iface-bridge
The error messages did not correspond to the attributes they printed.
---
tools/virsh-interface.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c
index 9ceb122..3be6d60 100644
--- a/tools/virsh-interface.c
+++ b/tools/virsh-interface.c
@@ -813,13 +813,13 @@ cmdInterfaceBridge(vshControl *ctl, const vshCmd *cmd)
*/
if (!xmlSetProp(if_node, BAD_CAST "type", BAD_CAST if_type)) {
vshError(ctl, _("Failed to set new slave interface type to '%s' in
xml document"),
- if_name);
+ if_type);
goto cleanup;
}
if (!xmlSetProp(if_node, BAD_CAST "name", BAD_CAST if_name)) {
vshError(ctl, _("Failed to set new slave interface name to '%s' in
xml document"),
- br_name);
+ if_name);
goto cleanup;
}
--
1.7.8.6
8:42 a.m.
New subject: [libvirt] [PATCH 5/7] virsh: fix error messages in iface-bridge
On 2012年11月28日 21:34, Ján Tomko wrote:
The error messages did not correspond to the attributes they
printed.
---
tools/virsh-interface.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c
index 9ceb122..3be6d60 100644
--- a/tools/virsh-interface.c
+++ b/tools/virsh-interface.c
@@ -813,13 +813,13 @@ cmdInterfaceBridge(vshControl *ctl, const vshCmd *cmd)
*/
if (!xmlSetProp(if_node, BAD_CAST "type", BAD_CAST if_type)) {
vshError(ctl, _("Failed to set new slave interface type to '%s' in
xml document"),
- if_name);
+ if_type);
goto cleanup;
}
if (!xmlSetProp(if_node, BAD_CAST "name", BAD_CAST if_name)) {
vshError(ctl, _("Failed to set new slave interface name to '%s' in
xml document"),
- br_name);
+ if_name);
goto cleanup;
}
ACK
8:43 a.m.
New subject: [libvirt] [PATCH 5/7] virsh: fix error messages in iface-bridge
On 2012年11月28日 22:42, Osier Yang wrote:
On 2012年11月28日 21:34, Ján Tomko wrote:
> The error messages did not correspond to the attributes they printed.
> ---
> tools/virsh-interface.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c
> index 9ceb122..3be6d60 100644
> --- a/tools/virsh-interface.c
> +++ b/tools/virsh-interface.c
> @@ -813,13 +813,13 @@ cmdInterfaceBridge(vshControl *ctl, const vshCmd
> *cmd)
> */
> if (!xmlSetProp(if_node, BAD_CAST "type", BAD_CAST if_type)) {
> vshError(ctl, _("Failed to set new slave interface type to '%s' in xml
> document"),
> - if_name);
> + if_type);
> goto cleanup;
> }
>
> if (!xmlSetProp(if_node, BAD_CAST "name", BAD_CAST if_name)) {
> vshError(ctl, _("Failed to set new slave interface name to '%s' in xml
> document"),
> - br_name);
> + if_name);
With the indentions fixed.
> goto cleanup;
> }
>
ACK
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
7:34 a.m.
New subject: [libvirt] [PATCH 6/7] conf: check the return value of virXPathNodeSet
In a few places, the return value could get passed to VIR_ALLOC_N without
being checked, resulting in a request to allocate a lot of memory if the
return value was negative.
---
src/conf/domain_conf.c | 8 ++++++--
src/conf/storage_conf.c | 4 +++-
2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 2ca608f..814859a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3258,7 +3258,9 @@ virSecurityLabelDefsParseXML(virDomainDefPtr def,
saved_node = ctxt->node;
/* Allocate a security labels based on XML */
- if ((n = virXPathNodeSet("./seclabel", ctxt, &list)) == 0)
+ if ((n = virXPathNodeSet("./seclabel", ctxt, &list)) < 0)
+ goto error;
+ if (n == 0)
return 0;
if (VIR_ALLOC_N(def->seclabels, n) < 0) {
@@ -3345,7 +3347,9 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDefPtr
**seclabels_rtn,
virSecurityLabelDefPtr vmDef = NULL;
char *model, *relabel, *label;
- if ((n = virXPathNodeSet("./seclabel", ctxt, &list)) == 0)
+ if ((n = virXPathNodeSet("./seclabel", ctxt, &list)) < 0)
+ goto error;
+ if (n == 0)
return 0;
if (VIR_ALLOC_N(seclabels, n) < 0) {
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 5a2cf1b..feeba17 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -510,7 +510,9 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt,
VIR_FREE(format);
}
- source->nhost = virXPathNodeSet("./host", ctxt, &nodeset);
+ if ((i = virXPathNodeSet("./host", ctxt, &nodeset)) < 0)
+ goto cleanup;
+ source->nhost = i;
if (source->nhost) {
if (VIR_ALLOC_N(source->hosts, source->nhost) < 0) {
--
1.7.8.6
9:08 a.m.
New subject: [libvirt] [PATCH 6/7] conf: check the return value of virXPathNodeSet
On 2012年11月28日 21:34, Ján Tomko wrote:
In a few places, the return value could get passed to VIR_ALLOC_N
without
being checked, resulting in a request to allocate a lot of memory if the
return value was negative.
Isn't it expected to fail? calloc fails that on Linux, but not sure what
the behaviour on other platform.
---
src/conf/domain_conf.c | 8 ++++++--
src/conf/storage_conf.c | 4 +++-
2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 2ca608f..814859a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3258,7 +3258,9 @@ virSecurityLabelDefsParseXML(virDomainDefPtr def,
saved_node = ctxt->node;
/* Allocate a security labels based on XML */
- if ((n = virXPathNodeSet("./seclabel", ctxt,&list)) == 0)
+ if ((n = virXPathNodeSet("./seclabel", ctxt,&list))< 0)
+ goto error;
+ if (n == 0)
return 0;
but anyway, this is good fix, even calloc will fails, the error just
points to the wrong direction.
if (VIR_ALLOC_N(def->seclabels, n)< 0) {
@@ -3345,7 +3347,9 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDefPtr
**seclabels_rtn,
virSecurityLabelDefPtr vmDef = NULL;
char *model, *relabel, *label;
- if ((n = virXPathNodeSet("./seclabel", ctxt,&list)) == 0)
+ if ((n = virXPathNodeSet("./seclabel", ctxt,&list))< 0)
+ goto error;
+ if (n == 0)
return 0;
if (VIR_ALLOC_N(seclabels, n)< 0) {
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 5a2cf1b..feeba17 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -510,7 +510,9 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt,
VIR_FREE(format);
}
- source->nhost = virXPathNodeSet("./host", ctxt,&nodeset);
+ if ((i = virXPathNodeSet("./host", ctxt,&nodeset))< 0)
+ goto cleanup;
+ source->nhost = i;
I'd like have a new var like "n", instead of reusing "i", which
is used for loop index in convention.
if (source->nhost) {
if (VIR_ALLOC_N(source->hosts, source->nhost)< 0) {
ACK. I will use "n" when pushing.
Osier
7:34 a.m.
New subject: [libvirt] [PATCH 7/7] conf: snapshot: check return value of virDomainSnapshotObjListNum
If it's negative, this might result in a request to allocate lots of
memory.
---
src/conf/snapshot_conf.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 72bdd30..06be34d 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -1026,7 +1026,7 @@ virDomainListSnapshots(virDomainSnapshotObjListPtr snapshots,
int ret = -1;
int i;
- if (!snaps)
+ if (!snaps || count < 0)
return count;
if (VIR_ALLOC_N(names, count) < 0 ||
VIR_ALLOC_N(list, count + 1) < 0) {
--
1.7.8.6
8:39 a.m.
New subject: [libvirt] [PATCH 7/7] conf: snapshot: check return value of virDomainSnapshotObjListNum
On 11/28/12 14:34, Ján Tomko wrote:
If it's negative, this might result in a request to allocate lots
of
memory.
---
src/conf/snapshot_conf.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
ACK. I'll push it later today.
Peter
9:56 a.m.
On 2012年11月28日 21:34, Ján Tomko wrote:
Ján Tomko (7):
nwfilter: fix NULL pointer check in virNWFilterSnoopReqNew
conf: prevent crash with no uuid in cephx auth secret
cgroup: fix impossible overrun in virCgroupAddTaskController
libssh2_session: support DSS keys as well
virsh: fix error messages in iface-bridge
conf: check the return value of virXPathNodeSet
conf: snapshot: check return value of virDomainSnapshotObjListNum
Okay, pushed except 2/7, and a bit changes on 6/7, 5/7, and 1/7.
Regards,
Osier
4406
days inactive
4411
days old
24 comments
4 participants
participants (4)
-
Eric Blake -
Ján Tomko -
Osier Yang -
Peter Krempa