From: Peter Dinda <pdinda@northwestern.edu>
Date: Mon, 31 Aug 2015 22:21:34 +0000 (-0500)
Subject: Cleanup and sanity-checking of use of strncpy/strcpy (Coverity static analysis)
X-Git-Url: http://v3vee.org/palacios/gitweb/gitweb.cgi?p=palacios.git;a=commitdiff_plain;h=d22c11cec4e8c3390bfe6bf16ed07f5d073f0d4a

Cleanup and sanity-checking of use of strncpy/strcpy (Coverity static analysis)
---

diff --git a/linux_module/iface-code-inject.c b/linux_module/iface-code-inject.c
index 1262d77..34c3e6a 100644
--- a/linux_module/iface-code-inject.c
+++ b/linux_module/iface-code-inject.c
@@ -74,7 +74,8 @@ static int vm_tophalf_inject (struct v3_guest * guest, unsigned int cmd, unsigne
 
     /* we have a binary name */
     if (top_arg.is_exec_hooked) {
-        strcpy(top->bin_file, top_arg.bin_file);
+        strncpy(top->bin_file, top_arg.bin_file,256);
+	top->bin_file[255] = 0;
         top->is_exec_hooked = 1;
         DEBUG("top->bin_file is %s\n", top->bin_file);
     } 
diff --git a/linux_module/iface-env-inject.c b/linux_module/iface-env-inject.c
index af8ff6d..af60915 100644
--- a/linux_module/iface-env-inject.c
+++ b/linux_module/iface-env-inject.c
@@ -68,7 +68,8 @@ static int vm_env_inject (struct v3_guest * guest, unsigned int cmd, unsigned lo
 
     env->num_strings = env_arg.num_strings;
     
-    strcpy(env->bin_name, env_arg.bin_name);
+    strncpy(env->bin_name, env_arg.bin_name, MAX_STRING_LEN);
+    env->bin_name[MAX_STRING_LEN-1] = 0;
     DEBUG("Binary hooked on: %s\n", env->bin_name);
 
     //DEBUG("Palacios: Allocating space for %u env var string ptrs...\n", env->num_strings);
diff --git a/linux_module/iface-file.c b/linux_module/iface-file.c
index e826296..92ed9ff 100644
--- a/linux_module/iface-file.c
+++ b/linux_module/iface-file.c
@@ -56,6 +56,7 @@ static int mkdir_recursive(const char * path, unsigned short perms) {
     }
 
     memset(tmp_str, 0, strlen(path) + 1);
+    // will terminate tmp_str
     strncpy(tmp_str, path, strlen(path));
 
     dirname_ptr = tmp_str;
@@ -226,7 +227,7 @@ static void * palacios_file_open(const char * path, int mode, void * private_dat
 	return NULL;
     }
 
-    pfile->path = palacios_alloc(strlen(path));
+    pfile->path = palacios_alloc(strlen(path) + 1);
     
     if (!pfile->path) { 
 	ERROR("Cannot allocate in file open\n");
@@ -234,7 +235,7 @@ static void * palacios_file_open(const char * path, int mode, void * private_dat
 	palacios_free(pfile);
 	return NULL;
     }
-    strncpy(pfile->path, path, strlen(path));
+    strncpy(pfile->path, path, strlen(path)); // will terminate pfile->path
     pfile->guest = guest;
     
     palacios_spinlock_init(&(pfile->lock));
diff --git a/linux_module/iface-host-dev.c b/linux_module/iface-host-dev.c
index c9c586e..c8f8f60 100644
--- a/linux_module/iface-host-dev.c
+++ b/linux_module/iface-host-dev.c
@@ -785,6 +785,7 @@ static v3_host_dev_t palacios_host_dev_open_deferred(char *url,
     memset(dev,0,sizeof(struct palacios_host_device_user));
     
     strncpy(dev->url,url,MAX_URL);
+    dev->url[MAX_URL-1] = 0;
     
     dev->guestdev = gdev;
     
diff --git a/linux_module/iface-host-pci.c b/linux_module/iface-host-pci.c
index 48097d8..9eb3a9a 100644
--- a/linux_module/iface-host-pci.c
+++ b/linux_module/iface-host-pci.c
@@ -184,6 +184,7 @@ static int register_pci_hw_dev(unsigned int cmd, unsigned long arg) {
 
     
     strncpy(host_dev->name, hw_dev_arg.name, 128);
+    host_dev->name[127] = 0;
     host_dev->v3_dev.host_data = host_dev;
     
 
diff --git a/linux_module/iface-keyed-stream.c b/linux_module/iface-keyed-stream.c
index 5bce431..365a9c7 100644
--- a/linux_module/iface-keyed-stream.c
+++ b/linux_module/iface-keyed-stream.c
@@ -310,7 +310,7 @@ static v3_keyed_stream_t open_stream_mem(char *url,
 		    return 0;
 		}
 
-		strcpy(mykey,url+4);
+		strcpy(mykey,url+4); // will fit
 		
 		mks = (struct mem_keyed_stream *) palacios_alloc(sizeof(struct mem_keyed_stream));
 
@@ -382,7 +382,7 @@ static v3_keyed_stream_key_t open_key_mem(v3_keyed_stream_t stream,
 	    return 0;
 	}
 
-	strcpy(mykey,key);
+	strcpy(mykey,key); // will fit
 
 	m = create_mem_stream();
 	
@@ -431,7 +431,7 @@ static void preallocate_hint_key_mem(v3_keyed_stream_t stream,
 	    return;
 	}
 	
-	strcpy(mykey,key);
+	strcpy(mykey,key); // will fit
        
 	m = create_mem_stream_internal(size);
 	
@@ -684,7 +684,7 @@ static v3_keyed_stream_t open_stream_file(char *url,
 	return 0;
     }
     
-    strcpy(fks->path,url+5);
+    strcpy(fks->path,url+5); // will fit
     
     fks->stype=STREAM_FILE;
 
@@ -796,6 +796,7 @@ static v3_keyed_stream_key_t open_key_file(v3_keyed_stream_t stream,
 	ERROR("cannot allocate file keyed stream for key %s\n",key);
 	return 0;
     }
+    // this sequence will fit and terminate with a zero
     strcpy(path,fks->path);
     strcat(path,"/");
     strcat(path,key);
@@ -2082,7 +2083,7 @@ static v3_keyed_stream_key_t open_key_user(v3_keyed_stream_t stream, char *key)
 
     s->op->type = PALACIOS_KSTREAM_OPEN_KEY;
     s->op->buf_len = len;
-    strncpy(s->op->buf,key,len);
+    strncpy(s->op->buf,key,len); // will terminate buffer
 
     // enter with it locked
     if (do_request_to_response(s,&flags)) { 
diff --git a/linux_module/iface-pstate-ctrl.c b/linux_module/iface-pstate-ctrl.c
index 2864ea0..9b3e4bb 100644
--- a/linux_module/iface-pstate-ctrl.c
+++ b/linux_module/iface-pstate-ctrl.c
@@ -876,6 +876,7 @@ static int get_current_governor(char **buf, unsigned int cpu)
     }
 
     strncpy(govname, policy->governor->name, MAX_GOV_NAME_LEN);
+    govname[MAX_GOV_NAME_LEN-1] = 0;
 
     get_cpu_var(core_state).linux_governor = govname;
     put_cpu_var(core_state);
diff --git a/linux_module/iface-stream.c b/linux_module/iface-stream.c
index 36c6185..207f5f0 100644
--- a/linux_module/iface-stream.c
+++ b/linux_module/iface-stream.c
@@ -246,7 +246,8 @@ static void * palacios_stream_open(struct v3_stream * v3_stream, const char * na
     stream->guest = guest;
     stream->connected = 0;
 
-    strncpy(stream->name, name, STREAM_NAME_LEN - 1);
+    strncpy(stream->name, name, STREAM_NAME_LEN);
+    stream->name[STREAM_NAME_LEN-1] = 0;
 
     init_waitqueue_head(&(stream->user_poll_queue));
     palacios_spinlock_init(&(stream->lock));
diff --git a/linux_module/main.c b/linux_module/main.c
index b4c4637..12fa2fe 100644
--- a/linux_module/main.c
+++ b/linux_module/main.c
@@ -134,7 +134,8 @@ static long v3_dev_ioctl(struct file * filp,
 		goto out_err2;
 	    }	   
 
-	    strncpy(guest->name, user_image.name, 127);
+	    strncpy(guest->name, user_image.name, 128);
+	    guest->name[127] = 0;
 
 	    INIT_LIST_HEAD(&(guest->exts));
 
diff --git a/palacios/src/devices/generic.c b/palacios/src/devices/generic.c
index bf4adc4..23f00ce 100644
--- a/palacios/src/devices/generic.c
+++ b/palacios/src/devices/generic.c
@@ -691,6 +691,7 @@ static int generic_init(struct v3_vm_info * vm, v3_cfg_tree_t * cfg) {
     
     memset(state, 0, sizeof(struct generic_internal));
     strncpy(state->name,dev_id,MAX_NAME);
+    state->name[MAX_NAME-1] = 0;
 
     if (!forward) { 
 	state->forward_type=GENERIC_PHYSICAL;
diff --git a/palacios/src/devices/host_pci.c b/palacios/src/devices/host_pci.c
index b12d3a0..c7dc435 100644
--- a/palacios/src/devices/host_pci.c
+++ b/palacios/src/devices/host_pci.c
@@ -519,6 +519,7 @@ static int host_pci_init(struct v3_vm_info * vm, v3_cfg_tree_t * cfg) {
     
     state->pci_bus = pci;
     strncpy(state->name, dev_id, 32);
+    state->name[31] = 0;
 
 
     dev = v3_add_device(vm, dev_id, &dev_ops, state);
diff --git a/palacios/src/devices/host_pci_selpriv.c b/palacios/src/devices/host_pci_selpriv.c
index 1af5e86..ab84b50 100644
--- a/palacios/src/devices/host_pci_selpriv.c
+++ b/palacios/src/devices/host_pci_selpriv.c
@@ -958,6 +958,7 @@ static int host_pci_init(struct v3_vm_info * vm, v3_cfg_tree_t * cfg) {
     
     state->pci_bus = pci;
     strncpy(state->name, dev_id, 32);
+    state->name[31] = 0;
 
 
     dev = v3_add_device(vm, dev_id, &dev_ops, state);
diff --git a/palacios/src/devices/ide.c b/palacios/src/devices/ide.c
index e9a0dd1..bc42ea8 100644
--- a/palacios/src/devices/ide.c
+++ b/palacios/src/devices/ide.c
@@ -1990,7 +1990,8 @@ static int connect_fn(struct v3_vm_info * vm,
     }
 
     if (model_str != NULL) {
-	strncpy(drive->model, model_str, sizeof(drive->model) - 1);
+	strncpy(drive->model, model_str, sizeof(drive->model));
+	drive->model[sizeof(drive->model)-1] = 0;
     }
 
     if (strcasecmp(type_str, "cdrom") == 0) {
diff --git a/palacios/src/devices/netdisk.c b/palacios/src/devices/netdisk.c
index 403d9d5..8e0ec7f 100644
--- a/palacios/src/devices/netdisk.c
+++ b/palacios/src/devices/netdisk.c
@@ -291,6 +291,7 @@ static int disk_init(struct v3_vm_info * vm, v3_cfg_tree_t * cfg) {
     PrintDebug(vm, VCORE_NONE, "Registering Net disk at %s:%s disk=%s\n", ip_str, port_str, disk_tag);
 
     strncpy(disk->disk_name, disk_tag, sizeof(disk->disk_name));
+    disk->disk_name[sizeof(disk->disk_name)-1] = 0;
     disk->ip_addr = v3_inet_addr(ip_str);
     disk->port = atoi(port_str);
     disk->vm = vm;
diff --git a/palacios/src/devices/pci.c b/palacios/src/devices/pci.c
index 6583fb0..ab14422 100644
--- a/palacios/src/devices/pci.c
+++ b/palacios/src/devices/pci.c
@@ -1637,6 +1637,7 @@ struct pci_device * v3_pci_register_device(struct vm_device * pci,
     pci_dev->fn_num = fn_num;
 
     strncpy(pci_dev->name, name, sizeof(pci_dev->name));
+    pci_dev->name[sizeof(pci_dev->name)-1] = 0;
     pci_dev->vm = pci->vm;
     pci_dev->priv_data = priv_data;
 
diff --git a/palacios/src/devices/pci_front.c b/palacios/src/devices/pci_front.c
index 08d03fe..868a3ab 100644
--- a/palacios/src/devices/pci_front.c
+++ b/palacios/src/devices/pci_front.c
@@ -1019,6 +1019,7 @@ static int pci_front_init(struct v3_vm_info * vm, v3_cfg_tree_t * cfg)
     
     state->pci_bus = bus;
     strncpy(state->name, dev_id, 32);
+    state->name[31] = 0;
     
     if (!(dev = v3_add_device(vm, dev_id, &dev_ops, state))) { 
 	PrintError(vm, VCORE_NONE, "pci_front (%s): unable to add device\n",state->name);
diff --git a/palacios/src/devices/pci_passthrough.c b/palacios/src/devices/pci_passthrough.c
index dda9c80..5792535 100644
--- a/palacios/src/devices/pci_passthrough.c
+++ b/palacios/src/devices/pci_passthrough.c
@@ -793,6 +793,7 @@ static int passthrough_init(struct v3_vm_info * vm, v3_cfg_tree_t * cfg) {
     
     state->pci_bus = pci;
     strncpy(state->name, dev_id, 32);
+    state->name[31] = 0 ;
 
 
     dev = v3_add_device(vm, dev_id, &dev_ops, state);
diff --git a/palacios/src/extensions/ext_inspector.c b/palacios/src/extensions/ext_inspector.c
index 18e6a9c..db12f05 100644
--- a/palacios/src/extensions/ext_inspector.c
+++ b/palacios/src/extensions/ext_inspector.c
@@ -47,6 +47,7 @@ static int init_inspector(struct v3_vm_info * vm, v3_cfg_tree_t * cfg, void ** p
     memset(state, 0, sizeof(struct v3_inspector_state));
 
     strncpy(state->state_tree.name, "vm->name", 50);
+    state->state_tree.name[49] = 0;
     state->state_tree.subtree = 1;
 
     *priv_data = state;
diff --git a/palacios/src/gears/ext_process_environment.c b/palacios/src/gears/ext_process_environment.c
index ce2f135..3fcfb7b 100644
--- a/palacios/src/gears/ext_process_environment.c
+++ b/palacios/src/gears/ext_process_environment.c
@@ -238,7 +238,9 @@ static int v3_copy_chunk_vmm32(struct guest_info * core, const char ** argstrs,
         var_dump.argv[i] = tmpstr; 
 
         /* copy the string */
+	// this is guaranteed to alwys null terminate tmpstr
         strncpy(tmpstr, (char*)argvn, strlen((char*)argvn) + 1);
+
         i++;
         cursor += 4;
         bytes += strlen((char*)argvn) + 1;
@@ -254,6 +256,7 @@ static int v3_copy_chunk_vmm32(struct guest_info * core, const char ** argstrs,
 	    return -1;
 	}
 
+	// will always null-terminate tmpstr
         strncpy(tmpstr, argstrs[i], strlen(argstrs[j]) + 1);
         var_dump.argv[i] = tmpstr;
         bytes += strlen(argstrs[j]) + 1;
@@ -304,6 +307,7 @@ static int v3_copy_chunk_vmm32(struct guest_info * core, const char ** argstrs,
         var_dump.envp[i] = tmpstr;
 
         /* deepcopy the string */
+	// will always null-terminate tmpstr
         strncpy(tmpstr, (char*)envpn, strlen((char*)envpn) + 1);
         i++;    
         cursor += 4;
@@ -319,7 +323,7 @@ static int v3_copy_chunk_vmm32(struct guest_info * core, const char ** argstrs,
 	    PrintError(core->vm_info, core, "Cannot allocate temp string\n");
 	    return -1;
 	}
-
+	// will always null-terminate tmpstr
         strncpy(tmpstr, envstrs[j], strlen(envstrs[j]) + 1);
         var_dump.envp[i] = tmpstr;
         bytes += strlen(envstrs[j]) + 1;
@@ -516,6 +520,7 @@ static int v3_copy_chunk_vmm64(struct guest_info * core, const char ** argstrs,
         var_dump.argv[i] = tmpstr; 
 
         /* copy the string */
+	// will always null-terminate tmpstr
         strncpy(tmpstr, (char*)argvn, strlen((char*)argvn) + 1);
         i++;
         cursor += 8;
@@ -532,6 +537,7 @@ static int v3_copy_chunk_vmm64(struct guest_info * core, const char ** argstrs,
 	    return -1;
 	}
 
+	// will always null-terminate tmpstr
         strncpy(tmpstr, argstrs[j], strlen(argstrs[j]) + 1);
         var_dump.argv[i] = tmpstr;
         bytes += strlen(argstrs[j]) + 1;
@@ -583,6 +589,7 @@ static int v3_copy_chunk_vmm64(struct guest_info * core, const char ** argstrs,
         var_dump.envp[i] = tmpstr;
 
         /* deepcopy the string */
+	// will always null-terminate tmpstr
         strncpy(tmpstr, (char*)envpn, strlen((char*)envpn) + 1);
         i++;    
         cursor += 8;
@@ -598,7 +605,7 @@ static int v3_copy_chunk_vmm64(struct guest_info * core, const char ** argstrs,
 	    PrintError(core->vm_info, core, "Cannot allocate temp string\n");
 	    return -1;
 	}
-
+	// will always null-terminate tmpstr
         strncpy(tmpstr, envstrs[i], strlen(envstrs[j]) + 1);
         var_dump.envp[i] = tmpstr;
         bytes += strlen(envstrs[j]) + 1;
diff --git a/palacios/src/palacios/vmm.c b/palacios/src/palacios/vmm.c
index 57d567b..89b0173 100644
--- a/palacios/src/palacios/vmm.c
+++ b/palacios/src/palacios/vmm.c
@@ -339,7 +339,9 @@ struct v3_vm_info * v3_create_vm(void * cfg, void * priv_data, char * name, unsi
     }
 
     memset(vm->name, 0, 128);
-    strncpy(vm->name, name, 127);
+    strncpy(vm->name, name, 128);
+    vm->name[127] = 0;
+    
 
     if(v3_cpu_mapper_register_vm(vm) == -1) {
 
diff --git a/palacios/src/palacios/vmm_config.c b/palacios/src/palacios/vmm_config.c
index 7897f3e..9e73d11 100644
--- a/palacios/src/palacios/vmm_config.c
+++ b/palacios/src/palacios/vmm_config.c
@@ -229,6 +229,7 @@ static struct v3_config * parse_config(void * cfg_blob) {
 	V3_Print(VM_NONE, VCORE_NONE, "File index=%d id=%s\n", idx, id);
 
 	strncpy(file->tag, id, V3_MAX_TAG_LEN);
+	file->tag[V3_MAX_TAG_LEN-1] = 0 ;
 
 	if (version==0) { 
 	    struct file_hdr_v0 * hdr = &(files_v0->hdrs[idx]);
diff --git a/palacios/src/palacios/vmm_dev_mgr.c b/palacios/src/palacios/vmm_dev_mgr.c
index 0904ea1..45cb8d1 100644
--- a/palacios/src/palacios/vmm_dev_mgr.c
+++ b/palacios/src/palacios/vmm_dev_mgr.c
@@ -172,6 +172,7 @@ int v3_save_vm_devices(struct v3_vm_info * vm, struct v3_chkpt * chkpt) {
     list_for_each_entry(dev, &(mgr->dev_list), dev_link) {
         if (dev->ops->save) {
             strncpy(name_table + tbl_offset, dev->name, V3_MAX_DEVICE_NAME);
+	    *(name_table + tbl_offset + V3_MAX_DEVICE_NAME - 1) = 0;
             tbl_offset += V3_MAX_DEVICE_NAME;
             num_saved_devs++;
         }  else {
@@ -558,6 +559,7 @@ struct vm_device * v3_add_device(struct v3_vm_info * vm,
     INIT_LIST_HEAD(&(dev->res_hooks));
 
     strncpy(dev->name, name, 32);
+    dev->name[31] = 0;
     dev->ops = ops;
     dev->private_data = private_data;
 
diff --git a/palacios/src/palacios/vmm_multitree.c b/palacios/src/palacios/vmm_multitree.c
index 06ee0c4..6b9e659 100644
--- a/palacios/src/palacios/vmm_multitree.c
+++ b/palacios/src/palacios/vmm_multitree.c
@@ -67,6 +67,7 @@ struct v3_mtree * v3_mtree_create_node(struct v3_mtree * root, char * name) {
 
     memset(node, 0, sizeof(struct v3_mtree));
     strncpy(node->name, name, V3_MTREE_NAME_LEN);
+    node->name[V3_MTREE_NAME_LEN-1] = 0;
 
     if ((ret = __insert_mtree_node(root, node))) {
 	PrintError(VM_NONE, VCORE_NONE, "Insertion failure\n");
diff --git a/palacios/src/palacios/vmm_symmod.c b/palacios/src/palacios/vmm_symmod.c
index 82d2f35..1cb2033 100644
--- a/palacios/src/palacios/vmm_symmod.c
+++ b/palacios/src/palacios/vmm_symmod.c
@@ -195,6 +195,7 @@ static int symbol_hcall_handler(struct guest_info * core, hcall_id_t hcall_id, v
 	}
 
 	strncpy(new_symbol->name, sym_name, 256);
+	new_symbol->name[255] = 0;
 	new_symbol->linkage = tmp_symbol->value;
 
 	list_add(&(new_symbol->sym_node), &(symmod_state->v3_sym_list));