Cleanup and sanity-checking of integer overflow, null comparisons, dead code (Coverit...

diff --git a/linux_module/iface-host-dev.c b/linux_module/iface-host-dev.c
index c8f8f60..23adb35 100644 (file)
--- a/linux_module/iface-host-dev.c
+++ b/linux_module/iface-host-dev.c
@@ -369,6 +369,7 @@ static long host_dev_ioctl(struct file * fp, unsigned int val, unsigned long arg
            
            switch (op.type) { 
                case PALACIOS_HOST_DEV_USER_REQUEST_READ_GUEST: {
+                   // possible overflow here, but only if user is asking for too much...
                    void *temp = palacios_alloc(op.len);
 
                    DEEP_DEBUG_PRINT("palacios: hostdev: read guest\n");

diff --git a/linux_module/iface-keyed-stream.c b/linux_module/iface-keyed-stream.c
index a6fb960..99524e1 100644 (file)
--- a/linux_module/iface-keyed-stream.c
+++ b/linux_module/iface-keyed-stream.c
@@ -1853,6 +1853,7 @@ static long keyed_stream_ioctl_user(struct file * filp, unsigned int ioctl, unsi
             return -EFAULT;
         }
 
+       // overflow possible here for very large request
        if (resize_op(&(s->op),size-sizeof(struct palacios_user_keyed_stream_op))) {
            ERROR("unable to resize op in user key push response\n");
             palacios_spinlock_unlock_irqrestore(&(s->lock), flags);
@@ -1934,6 +1935,7 @@ int keyed_stream_connect_user(struct v3_guest *guest, unsigned int cmd, unsigned
        return -1;
     }
 
+    // overflow possible here, but only if this is a huge guest request (>4GB)
     url = palacios_alloc(len);
     
     if (!url) { 

diff --git a/linux_module/main.c b/linux_module/main.c
index 310c5a1..f69b269 100644 (file)
--- a/linux_module/main.c
+++ b/linux_module/main.c
@@ -122,6 +122,7 @@ static long v3_dev_ioctl(struct file * filp,
            guest->img_size = user_image.size;
 
            DEBUG("Palacios: Allocating kernel memory for guest image (%llu bytes)\n", user_image.size);
+           // overflow possible here, but only if guest image is probably to large for kernel anyway...
            guest->img = palacios_valloc(guest->img_size);
 
            if (!guest->img) {
@@ -409,7 +410,7 @@ static int read_guests_details(struct seq_file *s, void *v)
     
     
  out:
-    if (mem) { palacios_vfree(mem); }
+    if (mem) { palacios_vfree(mem); } // dead code but kept for clarity
     if (core) { palacios_vfree(core); }
     if (base) { palacios_vfree(base); }
     

diff --git a/linux_module/memtrack.c b/linux_module/memtrack.c
index b64ab29..66c0c03 100644 (file)
--- a/linux_module/memtrack.c
+++ b/linux_module/memtrack.c
@@ -99,6 +99,7 @@ static int memtrack_snap(struct v3_guest *guest,
     
     //INFO("num_cores=%u",num_cores);
 
+    // overflow possible here, but only for an insane number of cores
     if (!(user_snap=palacios_alloc(sizeof(v3_mem_track_snapshot) + num_cores * sizeof(struct v3_core_mem_track)))) {
        ERROR("palacios: cannot allocate memory for copying user snapshot\n");
        goto fail;

diff --git a/linux_module/mm.c b/linux_module/mm.c
index e4c374a..8eb8fc9 100644 (file)
--- a/linux_module/mm.c
+++ b/linux_module/mm.c
@@ -305,7 +305,6 @@ int palacios_init_mm( void ) {
                    INFO("Could not allocate initial memory block for node %d below 4GB\n", node_id);
                    if (!pgs) {
                        ERROR("Could not allocate initial memory block for node %d without restrictions\n", node_id);
-                       BUG_ON(!pgs);
                        palacios_deinit_mm();
                        return -1;
                    }

diff --git a/palacios/src/devices/qcowdisk.c b/palacios/src/devices/qcowdisk.c
index 910e9bd..6759682 100644 (file)
--- a/palacios/src/devices/qcowdisk.c
+++ b/palacios/src/devices/qcowdisk.c
@@ -275,7 +275,7 @@ static v3_qcow2_t *v3_qcow2_open(struct v3_vm_info* vm, char *path, int flags)
   
   res->fd = v3_file_open(vm, path, flags);
        
-  if (res->fd < 0) {
+  if (!res->fd) {
     ERROR("failed to open underlying file\n");
     goto clean_mem;
   }

diff --git a/palacios/src/devices/telnet_cons.c b/palacios/src/devices/telnet_cons.c
index 5f82993..b9080f7 100644 (file)
--- a/palacios/src/devices/telnet_cons.c
+++ b/palacios/src/devices/telnet_cons.c
@@ -413,16 +413,8 @@ static struct v3_device_ops dev_ops = {
 static int key_handler( struct cons_state * state, uint8_t ascii) {
     PrintDebug(VM_NONE, VCORE_NONE, "Character recieved: 0x%x\n", ascii);
 
-    // printable
-    if (ascii < 0x80) {
-       const struct key_code * key = &(ascii_to_key_code[ascii]);
 
-       if (deliver_scan_code(state, (struct key_code *)key) == -1) {
-           PrintError(VM_NONE, VCORE_NONE, "Could not deliver scan code to vm\n");
-           return -1;
-       }
-
-    } else if (ascii == ESC_CHAR) { // Escape Key
+    if (ascii == ESC_CHAR) { // Escape Key
        // This means that another 2 characters are pending
        // receive it and deliver accordingly
        char esc_seq[2] = {0, 0};
@@ -457,7 +449,16 @@ static int key_handler( struct cons_state * state, uint8_t ascii) {
        } else if (esc_seq[1] == 'D') {         // LEFT ARROW
            struct key_code left = { 0x4B, 0 };
            deliver_scan_code(state, &left);
+       }   
+
+    } else if (ascii < 0x80) { // printable
+       const struct key_code * key = &(ascii_to_key_code[ascii]);
+
+       if (deliver_scan_code(state, (struct key_code *)key) == -1) {
+           PrintError(VM_NONE, VCORE_NONE, "Could not deliver scan code to vm\n");
+           return -1;
        }
+
     } else {
        PrintError(VM_NONE, VCORE_NONE, "Invalid character received from network (%c) (code=%d)\n",
                   ascii, ascii);

diff --git a/palacios/src/palacios/svm_npt.h b/palacios/src/palacios/svm_npt.h
index 3094d3d..f0d5262 100644 (file)
--- a/palacios/src/palacios/svm_npt.h
+++ b/palacios/src/palacios/svm_npt.h
@@ -63,6 +63,8 @@ static int handle_svm_invalidate_nested_addr(struct guest_info * info, addr_t in
 #endif
 
     switch(mode) {
+       // Note that the dead code here (for other than LONG and PROTECTED
+       // is kept here for clarity and parallelism with other impls
        case REAL:
        case PROTECTED:
          return invalidate_addr_32(info, inv_addr, actual_start, actual_end);
@@ -95,6 +97,8 @@ static int handle_svm_invalidate_nested_addr_range(struct guest_info * info,
 #endif
 
     switch(mode) {
+       // dead code except for LONG and PROTECTED cases
+       // this is kept for clarity and parallelism with other implementations
        case REAL:
        case PROTECTED:
          return invalidate_addr_32_range(info, inv_addr_start, inv_addr_end, actual_start, actual_end);

diff --git a/palacios/src/palacios/vmm_cpu_mapper.c b/palacios/src/palacios/vmm_cpu_mapper.c
index 626f50f..2c7fe89 100644 (file)
--- a/palacios/src/palacios/vmm_cpu_mapper.c
+++ b/palacios/src/palacios/vmm_cpu_mapper.c
@@ -224,7 +224,7 @@ int default_mapper_admit(struct v3_vm_info *vm, unsigned int cpu_mask){
     vcore_id--;
     }
 
-    if (vcore_id >= 0) {
+    if (vcore_id >= 0) { // dead code...
        v3_stop_vm(vm);
        return -1;
     }

diff --git a/palacios/src/palacios/vmm_events.c b/palacios/src/palacios/vmm_events.c
index c4580ee..7402959 100644 (file)
--- a/palacios/src/palacios/vmm_events.c
+++ b/palacios/src/palacios/vmm_events.c
@@ -35,6 +35,7 @@ int v3_init_events(struct v3_vm_info * vm) {
        return -1;
     }
 
+    // dead code if there are no events, but this is correct
     for (i = 0; i < V3_EVENT_INVALID; i++) {
        INIT_LIST_HEAD(&(map->events[i]));
     }
@@ -46,6 +47,8 @@ int v3_deinit_events(struct v3_vm_info * vm) {
     struct v3_event_map * map = &(vm->event_map);
     int i = 0;
 
+
+    // dead code if there are no events, but this is correct
     for (i = 0; i < V3_EVENT_INVALID; i++) {
        if (!list_empty(&(map->events[i]))) {
            struct v3_notifier * tmp_notifier = NULL;

diff --git a/palacios/src/palacios/vmm_paging.c b/palacios/src/palacios/vmm_paging.c
index 85c0874..03a158a 100644 (file)
--- a/palacios/src/palacios/vmm_paging.c
+++ b/palacios/src/palacios/vmm_paging.c
@@ -792,7 +792,7 @@ int v3_drill_host_pt_32(struct guest_info * info, v3_reg_t host_cr3, addr_t vadd
        case PT_ENTRY_NOT_PRESENT:
            return -1;
        case PT_ENTRY_LARGE_PAGE:
-           if ((ret == callback(info, PAGE_4MB, vaddr, (addr_t)V3_VAddr((void *)host_pte_pa), host_pte_pa, private_data)) != 0) {
+           if ((ret = callback(info, PAGE_4MB, vaddr, (addr_t)V3_VAddr((void *)host_pte_pa), host_pte_pa, private_data)) != 0) {
                return (ret == -1) ? -1 : PAGE_4MB;
            }
            return 0;
@@ -842,7 +842,7 @@ int v3_drill_host_pt_32pae(struct guest_info * info, v3_reg_t host_cr3, addr_t v
                case PT_ENTRY_NOT_PRESENT:
                    return -1;
                case PT_ENTRY_LARGE_PAGE:
-                   if ((ret == callback(info, PAGE_2MB, vaddr, (addr_t)V3_VAddr((void *)host_pte_pa), host_pte_pa, private_data)) != 0) {
+                   if ((ret = callback(info, PAGE_2MB, vaddr, (addr_t)V3_VAddr((void *)host_pte_pa), host_pte_pa, private_data)) != 0) {
                        return (ret == -1) ? -1 : PAGE_2MB;
                    }
                    return 0;
@@ -897,7 +897,7 @@ int v3_drill_host_pt_64(struct guest_info * info, v3_reg_t host_cr3, addr_t vadd
                case PT_ENTRY_NOT_PRESENT:
                    return -1;
                case PT_ENTRY_LARGE_PAGE:
-                   if ((ret == callback(info, PAGE_1GB, vaddr, (addr_t)V3_VAddr((void *)host_pde_pa), host_pde_pa, private_data)) != 0) {
+                   if ((ret = callback(info, PAGE_1GB, vaddr, (addr_t)V3_VAddr((void *)host_pde_pa), host_pde_pa, private_data)) != 0) {
                        return (ret == -1) ? -1 : PAGE_1GB;
                    }
                    PrintError(info->vm_info, info, "1 Gigabyte Pages not supported\n");
@@ -912,7 +912,7 @@ int v3_drill_host_pt_64(struct guest_info * info, v3_reg_t host_cr3, addr_t vadd
                        case PT_ENTRY_NOT_PRESENT:
                            return -1;
                        case PT_ENTRY_LARGE_PAGE:
-                           if ((ret == callback(info, PAGE_2MB, vaddr, (addr_t)V3_VAddr((void *)host_pte_pa), host_pte_pa, private_data)) != 0) {
+                           if ((ret = callback(info, PAGE_2MB, vaddr, (addr_t)V3_VAddr((void *)host_pte_pa), host_pte_pa, private_data)) != 0) {
                                return (ret == -1) ? -1 : PAGE_2MB;
                            }
                            return 0;
@@ -977,7 +977,7 @@ int v3_drill_guest_pt_32(struct guest_info * info, v3_reg_t guest_cr3, addr_t va
                }
 
 
-               if ((ret == callback(info, PAGE_4MB, vaddr, large_page_va, large_page_pa, private_data)) != 0) {
+               if ((ret = callback(info, PAGE_4MB, vaddr, large_page_va, large_page_pa, private_data)) != 0) {
                    return (ret == -1) ? -1 : PAGE_4MB;
                }
                return 0;
@@ -1071,7 +1071,7 @@ int v3_drill_guest_pt_32pae(struct guest_info * info, v3_reg_t guest_cr3, addr_t
                                        large_page_va = 0;
                                    }
              
-                                   if ((ret == callback(info, PAGE_2MB, vaddr, large_page_va, large_page_pa, private_data)) != 0) {
+                                   if ((ret = callback(info, PAGE_2MB, vaddr, large_page_va, large_page_pa, private_data)) != 0) {
                                        return (ret == -1) ? -1 : PAGE_2MB;
                                    }
                                    return 0;
@@ -1166,7 +1166,7 @@ int v3_drill_guest_pt_64(struct guest_info * info, v3_reg_t guest_cr3, addr_t va
                                large_page_va = 0;
                            }
          
-                           if ((ret == callback(info, PAGE_1GB, vaddr, large_page_va, large_page_pa, private_data)) != 0) {
+                           if ((ret = callback(info, PAGE_1GB, vaddr, large_page_va, large_page_pa, private_data)) != 0) {
                                return (ret == -1) ? -1 : PAGE_1GB;
                            }
                            PrintError(info->vm_info, info, "1 Gigabyte Pages not supported\n");
@@ -1199,7 +1199,7 @@ int v3_drill_guest_pt_64(struct guest_info * info, v3_reg_t guest_cr3, addr_t va
                                            large_page_va = 0;
                                        }
              
-                                       if ((ret == callback(info, PAGE_2MB, vaddr, large_page_va, large_page_pa, private_data)) != 0) {
+                                       if ((ret = callback(info, PAGE_2MB, vaddr, large_page_va, large_page_pa, private_data)) != 0) {
                                            return (ret == -1) ? -1 : PAGE_2MB;
                                        }
                                        return 0;

diff --git a/palacios/src/palacios/vmm_swapping.c b/palacios/src/palacios/vmm_swapping.c
index e3095d0..a5e806b 100644 (file)
--- a/palacios/src/palacios/vmm_swapping.c
+++ b/palacios/src/palacios/vmm_swapping.c
@@ -163,7 +163,7 @@ int v3_init_swapping_vm(struct v3_vm_info *vm, struct v3_xml *config)
 
     // Can we allocate the file?
 
-    if ((vm->swap_state.swapfd = v3_file_open(vm,file, FILE_OPEN_MODE_READ | FILE_OPEN_MODE_WRITE | FILE_OPEN_MODE_CREATE))<0) {
+    if (!(vm->swap_state.swapfd = v3_file_open(vm,file, FILE_OPEN_MODE_READ | FILE_OPEN_MODE_WRITE | FILE_OPEN_MODE_CREATE))) {
        PrintError(vm,VCORE_NONE,"swapper: cannot open or create swap file\n");
        return -1;
     } else {
@@ -196,7 +196,7 @@ int v3_init_swapping_vm(struct v3_vm_info *vm, struct v3_xml *config)
        !strcasecmp(strategy,"next_fit") ? V3_SWAP_NEXT_FIT :
        !strcasecmp(strategy,"random") ? V3_SWAP_RANDOM :
        !strcasecmp(strategy,"lru") ? V3_SWAP_LRU :
-       !strcasecmp(strategy,"default") ? V3_SWAP_RANDOM :
+       !strcasecmp(strategy,"default") ? V3_SWAP_RANDOM :  // identical branches for clarity
        V3_SWAP_RANDOM;
 
     vm->swap_state.host_mem_size=alloc;