[PATCH] D29892: ar: add llvm-dlltool support

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lib/Object/COFFImportFile.cpp:584-585
+    if (E.isWeak()) {
+      std::vector<uint8_t> *WeakBuffer1 = new std::vector<uint8_t>();
+      std::vector<uint8_t> *WeakBuffer2 = new std::vector<uint8_t>();
+      Members.push_back(
----------------
martell wrote:
> ruiu wrote:
> > martell wrote:
> > > martell wrote:
> > > > ruiu wrote:
> > > > > ruiu wrote:
> > > > > > martell wrote:
> > > > > > > ruiu wrote:
> > > > > > > > Please don't use raw `new`s. You really want to avoid doing that, not only in this patch, but in other patches as well. You are leaking memory. I believe you are doing this because otherwise you would be accessing free'd memory, but leaking memory is also a bug which is not acceptable. Using `new` is not a solution for a use-after-free bug.
> > > > > > > Sorry about that Rui, I didn't even realise I used `new` here.
> > > > > > > This doesn't even need `new` because it is pushed back onto the vector.
> > > > > > > Updating the patch now. Also fixed and tested with mingw-w64.
> > > > > > Why do you have to pass buffers in the first place? Their lifetime is till the end of this block, so it seems unnecessary.
> > > > > Can you address this comment?
> > > > In general I am just following the style of the rest of the function.
> > > > 
> > > > std::vector<uint8_t> ImportDescriptor; -> createImportDescriptor
> > > > std::vector<uint8_t> NullImportDescriptor; -> createNullImportDescriptor
> > > > std::vector<uint8_t> NullThunk; -> createNullThunk
> > > > 
> > > > std::vector<uint8_t> WeakBuffer1;
> > > > std::vector<uint8_t> WeakBuffer2;
> > > > 
> > > > Maybe a suggestion of a better name?
> > > > Unless you want a change to the signature of createWeakExternal?
> > > I would assume the others use std::vector<uint8_t> in the case that someone wants to use this function on a premade buffer of their own. Possible Public API???
> > > I am happy to put the buffer inside the createWeakExternal function itself if you want?
> > I don't think it's about style. Why do you have to pass WeakBuffer1 and WeakBuffer2? If there's no need, please don't do that.
> Gotcha, It has been awhile since I wrote this so I had to take some time to re read everything.
> You have to pass WeakBuffer1 and WeakBuffer2 because you need to have the buffers in memory up until the `writeArchive` function is called.
> By having the buffer in the function itself combined with the fact we call the function twice means we are using the same memory space for both buffers which obviously creates problems like having the same import created over and over.
> 
> `md5sum e15c2aae621caebae2f1cc91a040cf62  /martell/llvm/usr/x86_64-w64-mingw32/lib/libmsvcrt.a`
> vs
> `md5sum 6cfae6b5ba015594e4b3ba8f3ba9bde0  /martell/llvm/usr/x86_64-w64-mingw32/lib/libmsvcrt.a`
> 
> I will clarify what I mean by style.
> `ObjectFactory` has a `BumpPtrAllocator` so we could create the buffer from within the function like `createShortImport` but all the other functions that create full sections like `createNullImportDescriptor` `createNullImportDescriptor` and `createNullThunk` all use `std::vector<uint8_t>`
> For the record all of these functions could also use the `BumpPtrAllocator` I just assume they don't because it is cleaner and more readable which is why I done the same for `createWeakExternal`
When `writeArchive` is called, both WeakBuffer1 and WeakBuffer2 are dead because their lifetime ends at the end of their enclosing innermost block. (i.e. they are invalid after line 591.) That means this patch has a use-after-free bug. 


Repository:
  rL LLVM

https://reviews.llvm.org/D29892