[llvm-commits] [lld] r167245 - /lld/trunk/lib/ReaderWriter/MachO/WriterMachO.cpp

[Nick Kledzik]

On Nov 1, 2012, at 1:38 PM, Michael Spencer wrote:

> On Thu, Nov 1, 2012 at 1:15 PM, Nick Kledzik <kledzik at apple.com> wrote:
>> 
>> On Nov 1, 2012, at 12:46 PM, Michael J. Spencer wrote:
>>> Author: mspencer
>>> Date: Thu Nov  1 14:46:06 2012
>>> New Revision: 167245
>>> 
>>> URL: http://llvm.org/viewvc/llvm-project?rev=167245&view=rev
>>> Log:
>>> [MachO] Fix use after free.
>>> 
>>> Modified:
>>>   lld/trunk/lib/ReaderWriter/MachO/WriterMachO.cpp
>>> 
>>> Modified: lld/trunk/lib/ReaderWriter/MachO/WriterMachO.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/MachO/WriterMachO.cpp?rev=167245&r1=167244&r2=167245&view=diff
>>> ==============================================================================
>>> --- lld/trunk/lib/ReaderWriter/MachO/WriterMachO.cpp (original)
>>> +++ lld/trunk/lib/ReaderWriter/MachO/WriterMachO.cpp Thu Nov  1 14:46:06 2012
>>> @@ -206,6 +206,7 @@
>>>  dyld_info_command           *_dyldInfoLoadCommand;
>>>  std::vector<load_command*>   _loadCmds;
>>>  std::vector<ChunkSegInfo>    _sectionInfo;
>>> +  llvm::StringMap<uint32_t> dylibNamesToOrdinal;
>>> };
>>> 
>>> 
>>> @@ -774,7 +775,6 @@
>>>  this->addLoadCommand(new dylinker_command("/usr/lib/dyld", is64));
>>> 
>>>  // Add dylib load commands.
>>> -  llvm::StringMap<uint32_t> dylibNamesToOrdinal;
>>>  for (const SharedLibraryAtom* shlibAtom : file.sharedLibrary() ) {
>>>    StringRef installName = shlibAtom->loadName();
>>>    if ( dylibNamesToOrdinal.count(installName) == 0 ) {
>> 
>> Where was dylibNamesToOrdinal used outside the method where it is a local?
>> 
>> -Nick
> 
> Ah, I should have included that in the message.
> 
> Line 785:
> 
>  for (llvm::StringMap<uint32_t>::iterator it=dylibNamesToOrdinal.begin(),
>                            end=dylibNamesToOrdinal.end(); it != end; ++it) {
>    this->addLoadCommand(new dylib_command(it->first(), is64));
>  }
> 
> dylib_command takes a StringRef. It is later copied on line 496.
The string being referenced is the loadName of some SharedLibraryAtom.  The atom owns the string.  Some File owns the atom.  The File should not be deleted until after the Writer is done.  

Ah.  The problem is the use of llvm::StringMap which makes a copy of the string and then vends that copy.   Too bad there is no predefined DenseMapInfo for StringRef so I could just just use llvm::DenseMap<StringRef, uint32_t>.  


> 
> Also note that dylib_command is never deleted. Same for all the
> commands. They really should be using unique_ptr or a bumpptr
> allocator.
I'm reworking the load command stuff to be usable for reading and writing load commands.  I'll fix the allocation issue as part of that rewrite.

-Nick