CTSRD-CHERI / chericat

Other
2 stars 0 forks source link

Memory leak in a loop in scan_mem function #44

Open eupharina opened 2 months ago

eupharina commented 2 months ago

While reviewing static analysis reports on some CheriBSD ports, I came across this code:

        char *query_value;
        asprintf(&query_value, "(\"0x%lx\", \"0x%lx\", \"%s\", %d, %d, %d, %d)", 
                kivp->kve_start,
                kivp->kve_end,
                mmap_path,
                compart_id,
                kivp->kve_protection,
                kivp->kve_flags,
                kivp->kve_type);

        if (i == 0) {
                        // CSA warning below: Allocator sizeof operand mismatch
            insert_vm_query_values = (char*)malloc(sizeof(query_value)); 
            assert(insert_vm_query_values != NULL);

            insert_vm_query_values = strdup(query_value);
        } else {
            char *temp;
            asprintf(&temp, "%s,%s", 
                insert_vm_query_values,
                query_value);
            insert_vm_query_values = strdup(temp);
            free(temp);
        }
        free(query_value);

I don't have much context of what is going on here, but this looks like a memory leak in a loop to me: insert_vm_query_values is overwritten (both in i==0 and else branches) without freeing it's previous value. It's probably a good idea to rewrite this bit of code anyway with fewer string copies.

eupharina commented 2 months ago

Similar issues:

  1. https://github.com/CTSRD-CHERI/chericat/blob/a69d6b8af9c325a861fe5d438f3b7eb34dfaa187/src/cap_capture.c#L151C1-L166C17
  2. https://github.com/CTSRD-CHERI/chericat/blob/a69d6b8af9c325a861fe5d438f3b7eb34dfaa187/src/elf_utils.c#L232C1-L252C24
  3. https://github.com/CTSRD-CHERI/chericat/blob/a69d6b8af9c325a861fe5d438f3b7eb34dfaa187/src/vm_caps_view.c#L208
psjm3 commented 1 week ago

Thank you! Yes, the code was written pretty much in experimental mode, there are a few places I will need to refactor to fix a few bugs and for optimisation. The reason I haven't done much tidying up and optimisation yet is because I am still implementing the basic functionality, and there is a good chance a lot of the existing code will be removed once we figured out how the code should be structured (e.g. I have plans to change the command line options and libraries into pluggable modules, etc.).

But this could be a trivial fix, will definitely address them.