There are memory leaks and null pointer dereference bugs in the code.

[ErisianArchitect]

Describe the Bug
Earlier, someone posted an issue (#661) about a segfault that they were having. Upon further investigation, it was discovered that it was a null pointer dereference. I submitted a pull request (#662 ) to fix the error with verify, but then I went reading some more of the code and I noticed a pretty glaring flaw (or multiple).

/// Prints the content of the `Module` to a file.
pub fn print_to_file<P: AsRef<Path>>(&self, path: P) -> Result<(), LLVMString> {
    let path_str = path
        .as_ref()
        .to_str()
        .expect("Did not find a valid Unicode path string");
    let path = to_c_str(path_str);
    let mut err_string = MaybeUninit::uninit();
    let return_code = unsafe {
        LLVMPrintModuleToFile(
            self.module.get(),
            path.as_ptr() as *const ::libc::c_char,
            err_string.as_mut_ptr(),
        )
    };

    if return_code == 1 {
        unsafe {
            return Err(LLVMString::new(err_string.assume_init()));
        }
    }

    Ok(())
}

There are 2 problems with this code. The first problem is that LLVM might not even touch the err_string pointer, so it may be initialized to garbage, which could cause a segfault, or otherwise it might touch memory that it's not supposed to, producing garbage.

The second problem is that if return_code == 1 assumes that err_string has been properly initialized. There's no null pointer check.

I'm going to do some more digging to see if I can find more locations that have dereference errors or memory leaks, because I have a feeling that there are other places where this same mistake was made.

[my4ng]

From LLVM:

LLVMBool LLVMPrintModuleToFile(LLVMModuleRef M, const char *Filename,
 char **ErrorMessage) {
 std::error_code EC;
 raw_fd_ostream dest(Filename, EC, sys::fs::OF_TextWithCRLF);
 if (EC) {
 *ErrorMessage = strdup(EC.message().c_str());
 return true;
 }
 
 unwrap(M)->print(dest, nullptr);
 
 dest.close();
 
 if (dest.has_error()) {
 std::string E = "Error printing to file: " + dest.error().message();
 *ErrorMessage = strdup(E.c_str());
 return true;
 }
 
 return false;
}

If the returned value is 1, then the err_string will always be initialised to something, possibly the null pointer if strdup fails, so problem 1 isn't a problem. I agree there should be a null pointer check when initialising LLVMString, potentially panicking when calling LLVMString::new on a null pointer.

[ErisianArchitect]

I think that in cases where module verification would fail, the error string might not even be touched. So it could return 1, but not modify the string, resulting in a null pointer dereference.

I think the problem was that #661 was trying to print a module that was not fully formed and so failed verification. That's my best guess, at least.

[my4ng]

If the printing fails for whatever reason with return value 1, the error string will always be initialised as shown in the code snippet. The only case where the return value is 1 and the error message is null is when it fails to be malloc'ed.

[ErisianArchitect]

Hmm. Yeah, I didn't see the snippet before. I guess you edited it?

I also tested the code examples in #661, and I didn't encounter a segfault (with my version of LLVM). Maybe it's a problem with one of the versions. If only I knew what I did with my testing script I could test all the versions.

[my4ng]

Sorry I accidentally left out the second half of the snippet. I tested the code in #661 and verify/print_to_file but nothing segfaults for me either. Overall, I think the MayUninit is unneeded, but nothing critical, and there isn't nothing serious here either. Whatever's happening might be from somewhere else.

[ErisianArchitect]

I wonder if maybe it was an out of memory error.

[my4ng]

There are a lot of unnecessary and potentially UB Maybeuninit's that I will remove too.

[TheDan64]

I think the original assumption with MaybeUninit was that LLVM would always write to it, but that might not be the case

[ErisianArchitect]

I don't think that it is the case. I'm pretty sure MaybeUninit being uninitialized was the cause of the segfault for #661. But I can't be certain because I was unable to reproduce it.

[ErisianArchitect]

@my4ng , what version of LLVM did you get the snippet from? Was it LLVM 21.1.8?

[my4ng]

23.0.0git, it's in the link

[ErisianArchitect]

I took a look at the 21.x release branch, and it's the same code. So there wasn't any kind of change that happened between then and now.

I think the error was just from code == 1 && !err_str.is_null() followed by a dispose after creating the pointer with MaybeUninit. It was just uninitialized memory on the disposal branch causing either a segfault or a null pointer dereference error, depending on whether or not the memory was zeroed out.