labath wrote: The reason this code looks the way it does is that we've had crashes when trying to read corrupted elf files from memory, where we load the size of the elf file from the process memory, find that out that it's size is 935872395 GB, try to allocate a host buffer of that size, and then explode when the allocation failed.
Since the elf code wasn't really ready to work with files loaded from memory (it assumed section headers would always be present -- something which you're changing now), we removed the code which tries to dynamically size the buffer for reading the file. The only case we needed to support was reading the vdso image from memory (which worked because the vdso is a complete elf image with section headers and all, and it fits into a single page of memory), which is why we've added this workaround to determine the region size from the outside. This patch institutionalizes that workaround, but I'm unsure if that's the right direction for this. I'd like to understand where you want to take this afterwards. If the next step is to teach ObjectFileELF to read from the remaining memory regions (which I think you'll have to do, because most ELF files will not consist of a single memory region), then I think this change is unnecessary -- and maybe even harmful. Unnecessary, because if the code will know how to handle the additional memory regions, then it should be easy to teach it to extend the first region as well. Harmful, because if anything goes wrong (there is no ELF file at the place where we think it should be), then we could end up reading a huge block of memory for nothing (since the memory region data comes from a more trusted source, we shouldn't end up with the ridiculous values as we did before, but we could still end up reading several gigabytes of data only to find out that it doesn't actually represent an ELF file). So, I think it would be better to keep the current algorithm, which is to read a small chunk of memory -- just enough to check whether we're truly dealing with an object file, and leave the rest of the work to the object file plugin (if I'm not mistaken, that's the logic we use when reading from a file as well). Once the elf memory reading code gets sufficiently robust, we can remove the vdso GetMemoryRegion call as well. Or we can keep it as a performance optimization. We can see how it goes, but either way, I don't think that a GetMemoryRegion call should be necessary for loading of *other* object files. What do you make of all this? How were you planning to approach things? https://github.com/llvm/llvm-project/pull/123148 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
