[PATCH] D42043: c-index: CXString: fix MSAN read-past-end bug

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a reviewer: arphaman.
vsk added a comment.

In https://reviews.llvm.org/D42043#981409, @elsteveogrande wrote:

> Fixes, but first, a question for reviewers:
>
> Looking at the description of `clang_disposeString`:
>
>   /**
>    * \brief Free the given string.
>    */
>   CINDEX_LINKAGE void clang_disposeString(CXString string);
>   
>
> Does it seem incorrect to acquire some `const char *` pointer into this string, dispose the string, and then access the characters?


I'm inclined to say that this is incorrect, but skimming through the codebase it seems like we "get away" with this behavior all over the place.

> I've seen this happen a couple of times now.  As I make changes to my code I run into this pattern.  (Since now this triggers a use-after-free abort.)
> 
> I wanted to ask because, though it seemed obvious to me that this is incorrect usage, I'm now wondering if the expectation is that it's ok.

I've CC'd some Apple folks who can say more definitively how difficult it'd be to move away from this behavior. IMO it should be doable as long as there's some way to audit the codebase (e.g with ASan).

>   Or maybe wasn't technically ok, and we just "got away with it" before.  :)
>    
> 
> Anyway, assuming it's only correct to use the string before disposing it, then the fixes this time around are:
> 
> - Fix an ASAN use-after-free in `c-index-test` mentioned above.  Get the `CXString`, pass it around, then dispose when we're done with it.
> - Change `createEmpty` and `createNull` to delegate through `createRef`
> - `createRef` tolerates `nullptr` correctly.
> - I previously ran into ASAN aborts due to mixing malloc/free/operator new/delete.  I had changed everything to use operator new/delete.  However from C-land I can't `new char[length]`, only `malloc` (or `strdup` or whatever).  Change everything to malloc/free instead.


Repository:
  rC Clang

https://reviews.llvm.org/D42043