[libvirt] [PATCH 0/?] Various issues found by coverity
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
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
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.
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.
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
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
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
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
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
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
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
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
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.
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
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.
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
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
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
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
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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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
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
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
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
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
participants (4)
-
Eric Blake -
Ján Tomko -
Osier Yang -
Peter Krempa