clayborg 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.
How do you load the size of the ELF file from process memory? Are you saying the memory region information was corrupted in such cases? > > 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), They are not present in memory and never have been. So I changed the ELF code to not rely on requiring sections when the PT_DYNAMIC info or PT_NOTE info points to the same data without requiring section info. > 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). Right now reading 512 bytes means if we ask for program headers, some or all program headers might be returned as all PT_NULL program headers due to truncation and the way the parsing code is right now. This patch is really to get ELF core files to be able to load what it needs effectively from memory like the PT_DYNAMIC and entries in the dynamic section like: - dynamic symbol table - hash table for the symbol table - the DT_DEBUG entry for the shared library list And so we can access the PT_NOTE data so we can get to the GNU build ID information and anything else that is encoded into PT_LOAD[0]. If we stick to 512 bytes, it often isn't enough to get all the data for the program headers, and this was causing issues when we couldn't locate the shared library list in the dynamic loader using this ELF file, or get to the note data. > 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? This patch is needed before we can start reading all ELF files from memory when loading a core file. Right now if you don't specify an executable when loading a core file you get nothing. No shared libraries or anything. If a fix can be made to be able to correctly load ELF files from memory, we suddenly can not have a full image list with just a core file and no other executables because we will be able to correctly load them. We will also get the GNU build IDs from this memory ELF to ensure we don't allow someone to load the incorrect /usr/lib/libc.so from the current machine. If we can't figure out what the UUID was supposed to be, loading a core file often just loads what is on the current host and backtraces don't work at all due to using the wrong files. So with a new patch I really just want to ensure we can load all program header data and the things that the program headers point to for any PT_DYNAMIC and PT_NOTE stuff. The current object file stuff assumes we have the entire file mapped into memory if it is read from disk, so some accesses start failing if there are lots of program headers that exceed 512 bytes and also if the static data (not memory regions, but static data from the ELF file on disk in the first PT_LOAD[0] segment) they point to isn't available. Looking up the first memory region typically covers the contents of the `PT_LOAD[0]` section so it gathers all of the data. I don't get contiguous segments (I don't ask for PT_LOAD[0] + PT_LOAD[1] ...). So we can either do this in the ELF file by manually increasing the amount by grabbing the program headers and making sure we can load all program headers and all data they point to that we care and need like PT_DYNAMIC contents and PT_NOTE stuff, or we can just go with the first memory region. The first memory region is typically a page or two and contains all that we want. I am fine with either way, but this current methodoligy goes for the easy way and falls back to the previous 512 byte default if no memory region info is available. We can modify the ObjectFIleELF when in memory to be able to read subsequent regions correctly by special casing if we have a process. This already doesn't work when 512 bytes is all we get, so this patch will only make things better. If we parse all program headers and look for the PT_DYNAMIC and PT_NOTE segments, they can point to very large offsets within the ELF image, so if we try to do things intelligently, we might end up reading way too much. If the ELF file is corrupted in memory, same thing. The memory region seemed like the safest thing to do. But I am currently working with some Android folks and we do need to worry about loading ELF headers from the middle of an APK file. I am not sure how the memory regions look for shared libraries that are uncompressed inside of the APK files. If the memory regions are ok, then this patch would work, but if there is one memory region for the entire APK it wouldn't. Though I don't see how this can work if there are not valid memory regions for each shared library within the APK because they need different permissions. Let me know if you think I should try to intelligently resize within ObjectFileELF, or if anything I have said above made you change your mind. 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
