[cling] Try to avoid crashes in llvm::identify_magic #11174
Conversation
|
Starting build on |
|
FWIW I managed to make #include <llvm/ADT/Twine.h>
#include <llvm/BinaryFormat/Magic.h>
#include <llvm/Support/MemoryBuffer.h>
#include <iostream>
#include <unistd.h>
int main(int argc, char *argv[]) {
if (argc < 2) {
std::cerr << "Usage: " << argv[0] << " <file>" << std::endl;
return 1;
}
llvm::Twine Path(argv[1]);
auto FileOrError =
#ifdef FIXED
llvm::MemoryBuffer::getFile(Path, /*IsText=*/false,
/*RequiresNullTerminator=*/true,
/*IsVolatile=*/true);
#else
llvm::MemoryBuffer::getFile(Path, /*IsText=*/false,
/*RequiresNullTerminator=*/false);
#endif
if (!FileOrError) {
std::cerr << "Problem reading '" << argv[1] << "'!" << std::endl;
return 1;
}
std::unique_ptr<llvm::MemoryBuffer> FileBuffer = std::move(*FileOrError);
sleep(5);
llvm::file_magic Magic = llvm::identify_magic(FileBuffer->getBuffer());
std::cout << "Magic = " << Magic << std::endl;
return 0;
}Compile this into an executable, and then make a copy to use as input. While the program is sleeping, call |
|
Build failed on windows10/cxx14. |
what is the advantage over the solution in the reproducer you have (using llvm::MemoryBuffer::getFile with different arguments)? |
|
@hahnjo, this looks good to me but can we not fix the |
|
Starting build on |
It's plain C++ and we can guarantee that it's not using |
The question is whether |
|
Build failed on windows10/cxx14. |
|
Build failed on mac1015/cxx17. Failing tests: |
|
Starting build on |
I think this is a valid use-case. Moreover, we are planning to upstream that functionality from cling which suggests which library is needed on a missing symbol to the jit infrastructure. |
The overload taking a path opens the file and then mmap its contents. This can cause bus errors when another process truncates the file while we are trying to read it. Instead just read the first 1024 bytes, which should be enough for identify_magic to do its work.
|
Starting build on |
Ok, I've added a |
Note that we can fix it now, and backport the change. Otherwise I fear the |
We could, but I don't think this is the right approach: We're in the middle of an LLVM upgrade already and any change to LLVM prevents distributions from building against their vanilla LLVM libraries in the future. Moreover, I'm still not 100% convinced that it should be the original I can remove the |
|
Build failed on mac1015/cxx17. Failing tests: |
Thanks for the clarification. Let's keep it as it is now. |
|
Starting build on |
|
Build failed on ROOT-debian10-i386/soversion. Errors:
|
|
Build failed on windows10/cxx14. Errors:
|
|
Build failed on ROOT-performance-centos8-multicore/cxx17. Errors:
|
|
Build failed on ROOT-ubuntu2004/python3. Errors:
|
|
Build failed on mac1015/cxx17. Errors:
|
|
Build failed on mac11/cxx14. Errors:
|
|
Build failed on ROOT-ubuntu18.04/nortcxxmod. Errors:
|
4 participants
The overload taking a path opens the file and then
mmapits contents. This can cause bus errors when another process truncates the file while we are trying to read it. Instead just read the first 1024 bytes, which should be enough foridentify_magicto do its work.