TheDan64 / inkwell

It's a New Kind of Wrapper for Exposing LLVM (Safely)
https://thedan64.github.io/inkwell/
Apache License 2.0
2.38k stars 229 forks source link

Unsoundness in `MemoryBuffer::create_from_memory_range` #516

Open fuzzypixelz opened 4 months ago

fuzzypixelz commented 4 months ago

Describe the Bug

MemoryBuffer::create_from_memory_range calls LLVMCreateMemoryBufferWithMemoryRange with RequiresNullTerminator set to false, while an LLVM memory buffer should always be null-terminated:

https://github.com/llvm/llvm-project/blob/adacb5010f5ca6e923b3cf2d8ea47cbaab96099d/llvm/include/llvm/Support/MemoryBuffer.h#L41-L51

Please note that setting RequiresNullTerminator to true won't solve the issue; LLVM will simply throw an error. Instead, the function signature needs to be changed to require null-terminated buffers. There can be no zero-copy implementation of MemoryBuffer::create_from_memory_range for arbitrary &[u8].

The RequiresNullTerminator argument is truly baffling to me. It seems to mean "if true, check if the buffer is null-terminated" and not "I don't need this memory buffer to be null terminated" because it is always assumed to be so (perhaps you want to patch it after construction):

https://github.com/llvm/llvm-project/blob/main/llvm/lib/Support/MemoryBuffer.cpp#L49-L57

To Reproduce

Create a memory buffer with MemoryBuffer::create_from_memory_range and attempt to use it in Context::create_module_from_ir for instance. The parser will behave oddly as it tries to read past the buffer.

Expected Behavior

MemoryBuffer::create_from_memory_range should always pass a null-terminated buffer to LLVMCreateMemoryBufferWithMemoryRange and set RequiresNullTerminator to true.

As this requires a copy in the general case, it should take a type that represents a buffer b of length N where b[N - 1] == b'\0' and set the buffer length to N-1 in LLVMCreateMemoryBufferWithMemoryRange.

Otherwise the documentation should be updated to ask the user to do the the necessary manipulation themselves before calling the function.

LLVM Version (please complete the following information):

Desktop (please complete the following information):

Additional Context

N/A

TheDan64 commented 4 months ago

Dupe of #185 I think