[libvirt] [libvirt-glib 1/2] Use g_strlcpy instead of strncpy
This guarantees that the string will be nul-terminated. Coverity warned about this issue. --- libvirt-gobject/libvirt-gobject-connection.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libvirt-gobject/libvirt-gobject-connection.c b/libvirt-gobject/libvirt-gobject-connection.c index 91cc535..0525323 100644 --- a/libvirt-gobject/libvirt-gobject-connection.c +++ b/libvirt-gobject/libvirt-gobject-connection.c @@ -1583,7 +1583,7 @@ GVirNodeInfo *gvir_connection_get_node_info(GVirConnection *conn, } ret = g_slice_new(GVirNodeInfo); - strncpy (ret->model, info.model, sizeof (ret->model)); + g_strlcpy (ret->model, info.model, sizeof (ret->model)); ret->memory = info.memory; ret->cpus = info.cpus; ret->mhz = info.mhz; -- 1.8.0.2
fetch_list implementations in libvirt-gobject-connection.c and libvirt-gobject-storage-pool.c can misbehave in error situations or when the call is cancelled: - when the call is cancelled, 'lst' will be NULL and 'n' non-0 so we'll try to iterate over 'lst', which will cause a crash - when list_func fails, 'lst' is likely to be uninitialized, which will lead to invalid frees in the memory cleanup in the error: branch. We can avoid this issue by making sure 'lst' is initialized to 0 when it's created. --- libvirt-gobject/libvirt-gobject-connection.c | 10 ++++++---- libvirt-gobject/libvirt-gobject-storage-pool.c | 10 ++++++---- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/libvirt-gobject/libvirt-gobject-connection.c b/libvirt-gobject/libvirt-gobject-connection.c index 0525323..6888482 100644 --- a/libvirt-gobject/libvirt-gobject-connection.c +++ b/libvirt-gobject/libvirt-gobject-connection.c @@ -613,7 +613,7 @@ static gchar ** fetch_list(virConnectPtr vconn, if (g_cancellable_set_error_if_cancelled(cancellable, err)) goto error; - lst = g_new(gchar *, n); + lst = g_new0(gchar *, n); if ((n = list_func(vconn, lst, n)) < 0) { gvir_set_error(err, GVIR_CONNECTION_ERROR, 0, @@ -626,9 +626,11 @@ static gchar ** fetch_list(virConnectPtr vconn, return lst; error: - for (i = 0 ; i < n; i++) - g_free(lst[i]); - g_free(lst); + if (lst != NULL) { + for (i = 0 ; i < n; i++) + g_free(lst[i]); + g_free(lst); + } return NULL; } diff --git a/libvirt-gobject/libvirt-gobject-storage-pool.c b/libvirt-gobject/libvirt-gobject-storage-pool.c index 8f579a1..16b39d4 100644 --- a/libvirt-gobject/libvirt-gobject-storage-pool.c +++ b/libvirt-gobject/libvirt-gobject-storage-pool.c @@ -333,7 +333,7 @@ static gchar ** fetch_list(virStoragePoolPtr vpool, if (g_cancellable_set_error_if_cancelled(cancellable, err)) goto error; - lst = g_new(gchar *, n); + lst = g_new0(gchar *, n); if ((n = list_func(vpool, lst, n)) < 0) { gvir_set_error(err, GVIR_STORAGE_POOL_ERROR, 0, @@ -346,9 +346,11 @@ static gchar ** fetch_list(virStoragePoolPtr vpool, return lst; error: - for (i = 0 ; i < n; i++) - g_free(lst[i]); - g_free(lst); + if (lst != NULL) { + for (i = 0 ; i < n; i++) + g_free(lst[i]); + g_free(lst); + } return NULL; } -- 1.8.0.2
On 12/14/2012 05:03 PM, Christophe Fergeau wrote:
fetch_list implementations in libvirt-gobject-connection.c and libvirt-gobject-storage-pool.c can misbehave in error situations or when the call is cancelled: - when the call is cancelled, 'lst' will be NULL and 'n' non-0 so we'll try to iterate over 'lst', which will cause a crash - when list_func fails, 'lst' is likely to be uninitialized, which will lead to invalid frees in the memory cleanup in the error: branch. We can avoid this issue by making sure 'lst' is initialized to 0 when it's created. --- libvirt-gobject/libvirt-gobject-connection.c | 10 ++++++---- libvirt-gobject/libvirt-gobject-storage-pool.c | 10 ++++++---- 2 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/libvirt-gobject/libvirt-gobject-connection.c b/libvirt-gobject/libvirt-gobject-connection.c index 0525323..6888482 100644 --- a/libvirt-gobject/libvirt-gobject-connection.c +++ b/libvirt-gobject/libvirt-gobject-connection.c @@ -613,7 +613,7 @@ static gchar ** fetch_list(virConnectPtr vconn, if (g_cancellable_set_error_if_cancelled(cancellable, err)) goto error;
- lst = g_new(gchar *, n); + lst = g_new0(gchar *, n); if ((n = list_func(vconn, lst, n)) < 0) { gvir_set_error(err, GVIR_CONNECTION_ERROR, 0, @@ -626,9 +626,11 @@ static gchar ** fetch_list(virConnectPtr vconn, return lst;
error: - for (i = 0 ; i < n; i++) - g_free(lst[i]); - g_free(lst); + if (lst != NULL) { + for (i = 0 ; i < n; i++) + g_free(lst[i]); + g_free(lst); + } return NULL; }
diff --git a/libvirt-gobject/libvirt-gobject-storage-pool.c b/libvirt-gobject/libvirt-gobject-storage-pool.c index 8f579a1..16b39d4 100644 --- a/libvirt-gobject/libvirt-gobject-storage-pool.c +++ b/libvirt-gobject/libvirt-gobject-storage-pool.c @@ -333,7 +333,7 @@ static gchar ** fetch_list(virStoragePoolPtr vpool, if (g_cancellable_set_error_if_cancelled(cancellable, err)) goto error;
- lst = g_new(gchar *, n); + lst = g_new0(gchar *, n); if ((n = list_func(vpool, lst, n)) < 0) { gvir_set_error(err, GVIR_STORAGE_POOL_ERROR, 0, @@ -346,9 +346,11 @@ static gchar ** fetch_list(virStoragePoolPtr vpool, return lst;
error: - for (i = 0 ; i < n; i++) - g_free(lst[i]); - g_free(lst); + if (lst != NULL) { + for (i = 0 ; i < n; i++) + g_free(lst[i]); + g_free(lst); + } return NULL; }
I see you fixed it both ways (initialization to 0 as well as the check), so this should be definitely enough. ACK, Martin
Ping? Christophe On Fri, Dec 14, 2012 at 05:03:37PM +0100, Christophe Fergeau wrote:
This guarantees that the string will be nul-terminated. Coverity warned about this issue. --- libvirt-gobject/libvirt-gobject-connection.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libvirt-gobject/libvirt-gobject-connection.c b/libvirt-gobject/libvirt-gobject-connection.c index 91cc535..0525323 100644 --- a/libvirt-gobject/libvirt-gobject-connection.c +++ b/libvirt-gobject/libvirt-gobject-connection.c @@ -1583,7 +1583,7 @@ GVirNodeInfo *gvir_connection_get_node_info(GVirConnection *conn, }
ret = g_slice_new(GVirNodeInfo); - strncpy (ret->model, info.model, sizeof (ret->model)); + g_strlcpy (ret->model, info.model, sizeof (ret->model)); ret->memory = info.memory; ret->cpus = info.cpus; ret->mhz = info.mhz; -- 1.8.0.2
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Dec 19, 2012 at 01:59:42PM +0100, Christophe Fergeau wrote:
Ping?
Time for another ping ;)
On Fri, Dec 14, 2012 at 05:03:37PM +0100, Christophe Fergeau wrote:
This guarantees that the string will be nul-terminated. Coverity warned about this issue. --- libvirt-gobject/libvirt-gobject-connection.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libvirt-gobject/libvirt-gobject-connection.c b/libvirt-gobject/libvirt-gobject-connection.c index 91cc535..0525323 100644 --- a/libvirt-gobject/libvirt-gobject-connection.c +++ b/libvirt-gobject/libvirt-gobject-connection.c @@ -1583,7 +1583,7 @@ GVirNodeInfo *gvir_connection_get_node_info(GVirConnection *conn, }
ret = g_slice_new(GVirNodeInfo); - strncpy (ret->model, info.model, sizeof (ret->model)); + g_strlcpy (ret->model, info.model, sizeof (ret->model)); ret->memory = info.memory; ret->cpus = info.cpus; ret->mhz = info.mhz; -- 1.8.0.2
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 12/14/2012 05:03 PM, Christophe Fergeau wrote:
This guarantees that the string will be nul-terminated. Coverity warned about this issue. --- libvirt-gobject/libvirt-gobject-connection.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libvirt-gobject/libvirt-gobject-connection.c b/libvirt-gobject/libvirt-gobject-connection.c index 91cc535..0525323 100644 --- a/libvirt-gobject/libvirt-gobject-connection.c +++ b/libvirt-gobject/libvirt-gobject-connection.c @@ -1583,7 +1583,7 @@ GVirNodeInfo *gvir_connection_get_node_info(GVirConnection *conn, }
ret = g_slice_new(GVirNodeInfo); - strncpy (ret->model, info.model, sizeof (ret->model)); + g_strlcpy (ret->model, info.model, sizeof (ret->model)); ret->memory = info.memory; ret->cpus = info.cpus; ret->mhz = info.mhz;
OK, so nobody's having a look at this, so if I have the rights... ACK, Martin
On Wed, Jan 23, 2013 at 03:41:56PM +0100, Martin Kletzander wrote:
On 12/14/2012 05:03 PM, Christophe Fergeau wrote:
This guarantees that the string will be nul-terminated. Coverity warned about this issue. --- libvirt-gobject/libvirt-gobject-connection.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libvirt-gobject/libvirt-gobject-connection.c b/libvirt-gobject/libvirt-gobject-connection.c index 91cc535..0525323 100644 --- a/libvirt-gobject/libvirt-gobject-connection.c +++ b/libvirt-gobject/libvirt-gobject-connection.c @@ -1583,7 +1583,7 @@ GVirNodeInfo *gvir_connection_get_node_info(GVirConnection *conn, }
ret = g_slice_new(GVirNodeInfo); - strncpy (ret->model, info.model, sizeof (ret->model)); + g_strlcpy (ret->model, info.model, sizeof (ret->model)); ret->memory = info.memory; ret->cpus = info.cpus; ret->mhz = info.mhz;
OK, so nobody's having a look at this, so if I have the rights...
ACK,
Thanks for the review, I've pushed this now, Christophe
participants (2)
-
Christophe Fergeau -
Martin Kletzander