[libvirt] [PATCH] locking: avoid dereferencing a NULL pointer
From: Alex Jia <ajia@redhat.com> * src/locking/lock_driver_sanlock.c: in fact, virStrcpy calls virStrncpy(dest, src, strlen(src), destbytes) then return result, if 'path' is NULL, it means 'src' is also NULL, strlen(NULL) will dereference a NULL pointer, which probably causes a segmentation fault. Signed-off-by: Alex Jia <ajia@redhat.com> --- src/locking/lock_driver_sanlock.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 2d72510..8c6d873 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -158,7 +158,7 @@ static int virLockManagerSanlockSetupLockspace(void) memcpy(ls.name, VIR_LOCK_MANAGER_SANLOCK_AUTO_DISK_LOCKSPACE, SANLK_NAME_LEN); ls.host_id = 0; /* Doesn't matter for initialization */ ls.flags = 0; - if (!virStrcpy(ls.host_id_disk.path, path, SANLK_PATH_LEN)) { + if (!path || !virStrcpy(ls.host_id_disk.path, path, SANLK_PATH_LEN)) { virLockError(VIR_ERR_INTERNAL_ERROR, _("Lockspace path '%s' exceeded %d characters"), path, SANLK_PATH_LEN); -- 1.7.1
On 09/21/2011 12:10 PM, ajia@redhat.com wrote:
From: Alex Jia<ajia@redhat.com>
* src/locking/lock_driver_sanlock.c: in fact, virStrcpy calls virStrncpy(dest, src, strlen(src), destbytes) then return result, if 'path' is NULL, it means 'src' is also NULL, strlen(NULL) will dereference a NULL pointer, which probably causes a segmentation fault.
Signed-off-by: Alex Jia<ajia@redhat.com> --- src/locking/lock_driver_sanlock.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 2d72510..8c6d873 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -158,7 +158,7 @@ static int virLockManagerSanlockSetupLockspace(void) memcpy(ls.name, VIR_LOCK_MANAGER_SANLOCK_AUTO_DISK_LOCKSPACE, SANLK_NAME_LEN); ls.host_id = 0; /* Doesn't matter for initialization */ ls.flags = 0; - if (!virStrcpy(ls.host_id_disk.path, path, SANLK_PATH_LEN)) { + if (!path || !virStrcpy(ls.host_id_disk.path, path, SANLK_PATH_LEN)) { virLockError(VIR_ERR_INTERNAL_ERROR, _("Lockspace path '%s' exceeded %d characters"), path, SANLK_PATH_LEN);
NACK. The prior virAsprintf guarantees that path is non-NULL at this point. Rather, the real problem that Coverity is complaining about here is that the only way to get to the error_unlink: label is if path is already non-NULL, so that the 'if (path)' in that label is redundant. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 09/22/2011 04:12 AM, Eric Blake wrote:
On 09/21/2011 12:10 PM, ajia@redhat.com wrote:
From: Alex Jia<ajia@redhat.com>
* src/locking/lock_driver_sanlock.c: in fact, virStrcpy calls virStrncpy(dest, src, strlen(src), destbytes) then return result, if 'path' is NULL, it means 'src' is also NULL, strlen(NULL) will dereference a NULL pointer, which probably causes a segmentation fault.
Signed-off-by: Alex Jia<ajia@redhat.com> --- src/locking/lock_driver_sanlock.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 2d72510..8c6d873 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -158,7 +158,7 @@ static int virLockManagerSanlockSetupLockspace(void) memcpy(ls.name, VIR_LOCK_MANAGER_SANLOCK_AUTO_DISK_LOCKSPACE, SANLK_NAME_LEN); ls.host_id = 0; /* Doesn't matter for initialization */ ls.flags = 0; - if (!virStrcpy(ls.host_id_disk.path, path, SANLK_PATH_LEN)) { + if (!path || !virStrcpy(ls.host_id_disk.path, path, SANLK_PATH_LEN)) { virLockError(VIR_ERR_INTERNAL_ERROR, _("Lockspace path '%s' exceeded %d characters"), path, SANLK_PATH_LEN);
NACK. The prior virAsprintf guarantees that path is non-NULL at this point. Rather, the real problem that Coverity is complaining about here is that the only way to get to the error_unlink: label is if path is already non-NULL, so that the 'if (path)' in that label is redundant.
Yeah, you're right, I will commit a new patch, because this subject is inappropriate, so I haven't naming v2 for this patch, the new patch subject is "[libvirt] [PATCH] locking: remove redundant codes". Thanks, Alex
Detected by Coverity. The only way to get to error_unlink is if path was successfully assigned, so the if was useless. Meanwhile, there was a return statement that did not free path. * src/locking/lock_driver_sanlock.c (virLockManagerSanlockSetupLockspace): Fix mem-leak, and drop useless if. --- src/locking/lock_driver_sanlock.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 2d72510..13940f1 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -239,7 +239,7 @@ static int virLockManagerSanlockSetupLockspace(void) virReportSystemError(-rv, _("Unable to add lockspace %s"), path); - return -1; + goto error_unlink; } else { VIR_DEBUG("Lockspace %s is already registered", path); } @@ -250,8 +250,7 @@ static int virLockManagerSanlockSetupLockspace(void) return 0; error_unlink: - if (path) - unlink(path); + unlink(path); error: VIR_FORCE_CLOSE(fd); VIR_FREE(path); -- 1.7.4.4
On Wed, Sep 21, 2011 at 02:22:57PM -0600, Eric Blake wrote:
Detected by Coverity. The only way to get to error_unlink is if path was successfully assigned, so the if was useless. Meanwhile, there was a return statement that did not free path.
* src/locking/lock_driver_sanlock.c (virLockManagerSanlockSetupLockspace): Fix mem-leak, and drop useless if. --- src/locking/lock_driver_sanlock.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 2d72510..13940f1 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -239,7 +239,7 @@ static int virLockManagerSanlockSetupLockspace(void) virReportSystemError(-rv, _("Unable to add lockspace %s"), path); - return -1; + goto error_unlink; } else { VIR_DEBUG("Lockspace %s is already registered", path); } @@ -250,8 +250,7 @@ static int virLockManagerSanlockSetupLockspace(void) return 0;
error_unlink: - if (path) - unlink(path); + unlink(path); error: VIR_FORCE_CLOSE(fd); VIR_FREE(path);
ACK and pushed, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (4)
-
ajia＠redhat.com -
Alex Jia
-
Daniel Veillard -
Eric Blake