kvm tool: Make kvm structure to carry name copy

PATCH In review Cyrill Gorcunov 2 participants 6 messages Feb 3, 2012 – Feb 4, 2012 KVM

Share & tools

Cyrill Gorcunov

author

2012-02-03 19:57 UTC #0 lore raw

PATCH 1/1 kvm tool: Make kvm structure to carry name copy

If guest name is used (which is default case) the kvm might end up carrying the pointer to name which is allocated on stack.

kvm_cmd_run_init
(on stack) default_name
kvm__init(..., default_name)

    kvm->name = default_name

So I think better to allow kvm to carry own copy of guest name. 64 symbols should be more than enough.

Signed-off-by Cyrill Gorcunov <[email protected]>

---

I hope I didn't miss anything?

 tools/kvm/kvm.c                          |    2 +-
 tools/kvm/powerpc/include/kvm/kvm-arch.h |    2 +-
 tools/kvm/x86/include/kvm/kvm-arch.h     |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

tools/kvm/kvm.c +1 -1

Index: linux-2.6.git/tools/kvm/kvm.c
===================================================================
@@ -384,7 +384,7 @@ struct kvm *kvm__init(const char *kvm_de
 
 	kvm__arch_init(kvm, hugetlbfs_path, ram_size);
 
-	kvm->name = name;
+	strncpy(kvm->name, name, sizeof(kvm->name));
 
 	kvm_ipc__start(kvm__create_socket(kvm));
 	kvm_ipc__register_handler(KVM_IPC_PID, kvm__pid);

tools/kvm/powerpc/include/kvm/kvm-arch.h +1 -1

Index: linux-2.6.git/tools/kvm/powerpc/include/kvm/kvm-arch.h
===================================================================
@@ -64,7 +64,7 @@ struct kvm {
 	unsigned long		fdt_gra;
 	unsigned long		initrd_gra;
 	unsigned long		initrd_size;
-	const char		*name;
+	char			name[64];
 	int			vm_state;
 };

tools/kvm/x86/include/kvm/kvm-arch.h +1 -1

Index: linux-2.6.git/tools/kvm/x86/include/kvm/kvm-arch.h
===================================================================
@@ -48,7 +48,7 @@ struct kvm {
 	struct disk_image       **disks;
 	int                     nr_disks;
 
-	const char		*name;
+	char			name[64];
 
 	int			vm_state;
 };

Pekka Enberg

2012-02-04 12:15 UTC #1 lore raw

Quoted 8 lines · Cyrill Gorcunov

If guest name is used (which is default case) the kvm might end up carrying the pointer to name which is allocated on stack.
kvm_cmd_run_init
(on stack) default_name
kvm__init(..., default_name)
kvm->name = default_name

Quoted 7 lines

So I think better to allow kvm to carry own copy of guest name. 64 symbols should be more than enough.
Signed-off-by Cyrill Gorcunov <[email protected]>
---
I hope I didn't miss anything?

Can't we just use strdup()?

Pekka

Cyrill Gorcunov

author

2012-02-04 12:20 UTC #2 lore raw

Quoted 12 lines · Pekka Enberg

Quoted 11 lines · Cyrill Gorcunov
If guest name is used (which is default case) the kvm might end up carrying the pointer to name which is allocated on stack.
kvm_cmd_run_init
(on stack) default_name
kvm__init(..., default_name)
kvm->name = default_name
So I think better to allow kvm to carry own copy of guest name. 64 symbols should be more than enough.

Quoted 7 lines

Quoted 4 lines
Signed-off-by Cyrill Gorcunov <[email protected]>
---
I hope I didn't miss anything?
Can't we just use strdup()?

Yeah, I think this will be even better, I'll update.

Cyrill

Cyrill Gorcunov

author

2012-02-04 12:27 UTC #3 lore raw

Quoted 16 lines · Cyrill Gorcunov

Quoted 15 lines · Pekka Enberg
Quoted 14 lines · Cyrill Gorcunov
If guest name is used (which is default case) the kvm might end up carrying the pointer to name which is allocated on stack.
kvm_cmd_run_init
(on stack) default_name
kvm__init(..., default_name)
kvm->name = default_name
So I think better to allow kvm to carry own copy of guest name. 64 symbols should be more than enough.
Signed-off-by Cyrill Gorcunov <[email protected]>
---

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
I hope I didn't miss anything?
Can't we just use strdup()?
Yeah, I think this will be even better, I'll update.

Something like below I think.

Cyrill
---
Subject: [PATCH] kvm tool: Make kvm structure to carry name copy

If default guest name is used (which is the default case) the kvm might end up carrying the pointer to a name which is allocated on stack.

kvm_cmd_run_init
(on stack) default_name
kvm__init(..., default_name)

    kvm->name = default_name

So make it to carry a copy of name.

Signed-off-by Cyrill Gorcunov <[email protected]>

---

 tools/kvm/kvm.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

tools/kvm/kvm.c +7 -2

Index: linux-2.6.git/tools/kvm/kvm.c
===================================================================
@@ -254,6 +254,7 @@ int kvm__exit(struct kvm *kvm)
 	kvm__arch_delete_ram(kvm);
 	kvm_ipc__stop();
 	kvm__remove_socket(kvm->name);
+	free((void *)kvm->name);
 	free(kvm);
 
 	return 0;

@@ -377,6 +378,12 @@ struct kvm *kvm__init(const char *kvm_de
 		goto cleanup;
 	}
 
+	kvm->name = strdup(name);
+	if (!kvm->name) {
+		ret = -ENOMEM;
+		goto cleanup;
+	}
+
 	if (kvm__check_extensions(kvm)) {
 		pr_err("A required KVM extention is not supported by OS");
 		ret = -ENOSYS;

@@ -384,8 +391,6 @@ struct kvm *kvm__init(const char *kvm_de
 
 	kvm__arch_init(kvm, hugetlbfs_path, ram_size);
 
-	kvm->name = name;
-
 	kvm_ipc__start(kvm__create_socket(kvm));
 	kvm_ipc__register_handler(KVM_IPC_PID, kvm__pid);
 	return kvm;

Pekka Enberg

2012-02-04 12:57 UTC #4 lore raw

tools/kvm/kvm.c int kvm__exit(struct kvm *kvm) · line 257

Quoted 9 lines · Cyrill Gorcunov

Index: linux-2.6.git/tools/kvm/kvm.c
===================================================================
tools/kvm/kvm.c +1 -0
@@ -254,6 +254,7 @@ int kvm__exit(struct kvm *kvm)
 	kvm__arch_delete_ram(kvm);
 	kvm_ipc__stop();
 	kvm__remove_socket(kvm->name);
+	free((void *)kvm->name);

Please fix the struct definition and drop the cast.

Quoted 4 lines

tools/kvm/kvm.c +0 -0

 	free(kvm);
 
 	return 0;

@@ -377,6 +378,12 @@ struct kvm *kvm__init(const char *kvm_de

Cyrill Gorcunov

author

2012-02-04 13:08 UTC #5 lore raw

Quoted 13 lines · Pekka Enberg

tools/kvm/kvm.c int kvm__exit(struct kvm *kvm) · line 257
Quoted 9 lines · Cyrill Gorcunov
Index: linux-2.6.git/tools/kvm/kvm.c
===================================================================
tools/kvm/kvm.c +1 -0
@@ -254,6 +254,7 @@ int kvm__exit(struct kvm *kvm)
 	kvm__arch_delete_ram(kvm);
 	kvm_ipc__stop();
 	kvm__remove_socket(kvm->name);
+	free((void *)kvm->name);
Please fix the struct definition and drop the cast.

I believe having it as const char * might save us some potential troubles, but sure, here we go ;)

Cyrill
---
kvm tool: Make kvm structure to carry name copy

If default guest name is used (which is the default case) the kvm might end up carrying the pointer to a name which is allocated on stack.

kvm_cmd_run_init
(on stack) default_name
kvm__init(..., default_name)

    kvm->name = default_name

So make it to carry a copy of name.

Signed-off-by Cyrill Gorcunov <[email protected]>

---

 tools/kvm/kvm.c                          |    9 +++++++--
 tools/kvm/powerpc/include/kvm/kvm-arch.h |    2 +-
 tools/kvm/x86/include/kvm/kvm-arch.h     |    2 +-
 3 files changed, 9 insertions(+), 4 deletions(-)

tools/kvm/kvm.c +7 -2

Index: linux-2.6.git/tools/kvm/kvm.c
===================================================================
@@ -254,6 +254,7 @@ int kvm__exit(struct kvm *kvm)
 	kvm__arch_delete_ram(kvm);
 	kvm_ipc__stop();
 	kvm__remove_socket(kvm->name);
+	free(kvm->name);
 	free(kvm);
 
 	return 0;

@@ -377,6 +378,12 @@ struct kvm *kvm__init(const char *kvm_de
 		goto cleanup;
 	}
 
+	kvm->name = strdup(name);
+	if (!kvm->name) {
+		ret = -ENOMEM;
+		goto cleanup;
+	}
+
 	if (kvm__check_extensions(kvm)) {
 		pr_err("A required KVM extention is not supported by OS");
 		ret = -ENOSYS;

@@ -384,8 +391,6 @@ struct kvm *kvm__init(const char *kvm_de
 
 	kvm__arch_init(kvm, hugetlbfs_path, ram_size);
 
-	kvm->name = name;
-
 	kvm_ipc__start(kvm__create_socket(kvm));
 	kvm_ipc__register_handler(KVM_IPC_PID, kvm__pid);
 	return kvm;

tools/kvm/powerpc/include/kvm/kvm-arch.h +1 -1

Index: linux-2.6.git/tools/kvm/powerpc/include/kvm/kvm-arch.h
===================================================================
@@ -64,7 +64,7 @@ struct kvm {
 	unsigned long		fdt_gra;
 	unsigned long		initrd_gra;
 	unsigned long		initrd_size;
-	const char		*name;
+	char			*name;
 	int			vm_state;
 };

tools/kvm/x86/include/kvm/kvm-arch.h +1 -1

Index: linux-2.6.git/tools/kvm/x86/include/kvm/kvm-arch.h
===================================================================
@@ -48,7 +48,7 @@ struct kvm {
 	struct disk_image       **disks;
 	int                     nr_disks;
 
-	const char		*name;
+	char			*name;
 
 	int			vm_state;
 };