c-index: CXString: fix MSAN read-past-end bug

[elsteveogrande]

Summary

See discussion here:
http://clang-developers.42468.n3.nabble.com/RFC-proper-string-handling-in-CXString-td4059287.html

Bugzilla URL:
https://bugs.llvm.org/show_bug.cgi?id=35896

Previous impl would read the byte past the end of a string (a llvm::StringRef), possibly exceeding the allocation for that memory and raising an MSAN issue.

This will instead save the character range specified by the StringRef and copy it on-demand to a new C string.

Passes existing tests, and now passes with MSAN as well.

[elsteveogrande]

Change string-copy-on-demand logic; do only if not IsNullTerminated.

[elsteveogrande]

Add a needed null-check on input string's data ptr.

[vsk]

tools/c-index-test/c-index-test.c

3268

This looks like a separate bug fix. Is it possible to separate the main_filename changes from this patch?

tools/libclang/CXString.cpp

59–60

Why shouldn't this be defined as createRef("")?

213

Basic question: If a non-owning CXString is null-terminated, what provides the guarantee that the string is in fact valid when getCString() is called? Is the user of the C API responsible for ensuring the lifetime of the string is valid?

tools/libclang/CXString.h

108

Generally unrelated whitespace changes should be left out of patches.

[elsteveogrande]

Thanks very much for looking at this @vsk ! I actually found an ASAN bug in my new code, mixing and matching malloc/free and operators new/delete.

tools/c-index-test/c-index-test.c

3268

Will do!

tools/libclang/CXString.cpp

59–60

Hmm, good question. createRef below actually calls back to createEmpty and createNull in the nonnull-but-empty and null cases.

I think I'll do this the other way around, and let createRef have the responsibility of dealing with these fields and nulls and whatnot.

213

I believe the API itself is the one building CXString instances, and the user of the C API doesn't really create them, only use them. So the API has to ensure the string stays "good" while there may be references to it.

(Which feels a little fragile. But I think that's the tradeoff being made. You'll get either "fast" strings, or data guaranteed to be sane. I'd opt for safer data but I don't know who's using this C API and am afraid to introduce a serious perf regression. So it'll stay this way and I'll try my best to solve *-SAN issues with these constraints :) )

[vsk]

Thanks for working on this :).

tools/libclang/CXString.cpp

213

Sgtm, it doesn't look like this is altering the API contract.

[elsteveogrande]

tools/c-index-test/c-index-test.c

3268

Done: https://reviews.llvm.org/D42259

[vsk]

Mind rebasing this when you have a chance?

[elsteveogrande]

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'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. 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.

[elsteveogrande]

remove unneeded changes

[vsk]

In 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.

[elsteveogrande]

Hi @vsk + @arphaman : I did find such problems with either MSAN (with poison-in-dtor) and ASAN, so we should be able to discover these instances pretty easily :)

[vitalybuka]

Is this stale?

[elsteveogrande]

Rebase + fix conflicts for very old diff. Works again.

ninja check-clang with MSAN-enabled build:

Before:

Failing Tests (2):
    Clang :: CodeGen/signed_metadata.cpp
    Clang :: Index/comment-to-html-xml-conversion.cpp

  Expected Passes    : 12777
  Expected Failures  : 19
  Unsupported Tests  : 291
  Unexpected Failures: 2
FAILED: tools/clang/test/CMakeFiles/check-clang
cd /data/users/steveo/llvm-ct/build/msan/clang/tools/clang/test && /bin/python2.7 /data/users/steveo/llvm-ct/build/msan/clang/./bin/llvm-lit -sv --param clang_site_config=/data/users/steveo/llvm-ct/build/msan/clang/tools/clang/test/lit.site.cfg --param USE_Z3_SOLVER=0 /data/users/steveo/llvm-ct/build/msan/clang/tools/clang/test
ninja: build stopped: subcommand failed.

After:

Failing Tests (1):
    Clang :: CodeGen/signed_metadata.cpp

  Expected Passes    : 12778
  Expected Failures  : 19
  Unsupported Tests  : 291
  Unexpected Failures: 1
FAILED: tools/clang/test/CMakeFiles/check-clang
cd /data/users/steveo/llvm-ct/build/msan/clang/tools/clang/test && /bin/python2.7 /data/users/steveo/llvm-ct/build/msan/clang/./bin/llvm-lit -sv --param clang_site_config=/data/users/steveo/llvm-ct/build/msan/clang/tools/clang/test/lit.site.cfg --param USE_Z3_SOLVER=0 /data/users/steveo/llvm-ct/build/msan/clang/tools/clang/test
ninja: build stopped: subcommand failed.

[elsteveogrande]

hi @vsk + @arphaman , it's been a while -- I brought this old diff up to date, wondering if you could take a look again. Thanks!

[vsk]

+ Jan and Volodymyr. This seemed to be in good shape the last time I looked at it. Not having touched libclang for a while I don't think I can give an official lgtm.

[riccibruno]

Is anyone working on this ? This is the only persistent msan failure when doing check-clang on my machine.

include/clang-c/CXString.h

 |* This header provides the interface to C Index strings.                     *|
 |*                                                                            *|
 \*===----------------------------------------------------------------------===*/
 
 #ifndef LLVM_CLANG_C_CXSTRING_H
 #define LLVM_CLANG_C_CXSTRING_H
 
 #include "clang-c/Platform.h"
+#include <stdbool.h>
 
 #ifdef __cplusplus
 extern "C" {
 #endif
 
 /**
  * \defgroup CINDEX_STRING String manipulation routines
  * \ingroup CINDEX
  *
  * @{
  */
 
 /**
  * \brief A character string.
  *
  * The \c CXString type is used to return strings from the interface when
  * the ownership of that string might differ from one call to the next.
  * Use \c clang_getCString() to retrieve the string data and, once finished
  * with the string data, call \c clang_disposeString() to free the string.
  */
 typedef struct {
-  const void *data;
-  unsigned private_flags;
+  const void *Contents;
+  unsigned Size;
+  bool IsNullTerminated;
+  bool IsOwned;
+  bool IsPooled;
 } CXString;
 
 typedef struct {
   CXString *Strings;
   unsigned Count;
 } CXStringSet;
 
 /**
  * \brief Retrieve the character data associated with the given string.
+ *
+ * This returns a pointer to a C string even if this \b CXString was built
+ * from e.g. a StringRef reference (which is not null-terminated).  In those
+ * cases a string copy is performed on-demand.
+ *
+ * See also \b clang_getStringData and \b clang_getStringSize which could
+ * provide a range or "window" of data, if needed, and a C string is not
+ * strictly needed; this could save a C string allocation, deallocation, and
+ * string copy.
  */
 CINDEX_LINKAGE const char *clang_getCString(CXString string);
 
 /**
+ * \brief Retrieve a pointer to the start of the character data for this string.
+ *
+ * Note that the string is not necessarily null-terminated, so it might not be
+ * suitable for use as a C string.  See \b clang_getCString which guarantees
+ * a C string result.
+ */
+CINDEX_LINKAGE const char *clang_getStringData(CXString string);
+
+/**
+ * \brief Retrieve the string length (not including null-terminator, if any).
+ */
+CINDEX_LINKAGE unsigned clang_getStringSize(CXString string);
+
+/**
  * \brief Free the given string.
  */
 CINDEX_LINKAGE void clang_disposeString(CXString string);
 
 /**
  * \brief Free the given string set.
  */
 CINDEX_LINKAGE void clang_disposeStringSet(CXStringSet *set);
 
 /**
  * @}
  */
 
 #ifdef __cplusplus
 }
 #endif
 #endif
-

tools/c-index-test/c-index-test.c

static const char* GetCursorSource(CXCursor Cursor) {
   }
   else {
     const char *b = basename(clang_getCString(source));
     clang_disposeString(source);
     return b;
   }
 }
 
+static CXString createCXString(const char *CS) {
+  CXString Str;
+  Str.Contents = (const void *) CS;
+  Str.Size = strlen(CS);
+  Str.IsNullTerminated = true;
+  Str.IsOwned = false;
+  Str.IsPooled = false;
+  return Str;
+}
+
 /******************************************************************************/
 /* Callbacks.                                                                 */
 /******************************************************************************/
 
 typedef void (*PostVisitTU)(CXTranslationUnit);
 
 void PrintDiagnostic(CXDiagnostic Diagnostic) {
   FILE *out = stderr;
typedef struct IndexDataStringList_ {
   char data[1]; /* Dynamically sized. */
 } IndexDataStringList;
 
 typedef struct {
   const char *check_prefix;
   int first_check_printed;
   int fail_for_error;
   int abort;
-  const char *main_filename;
+  CXString main_filename;
   ImportedASTFilesData *importedASTs;
   IndexDataStringList *strings;
   CXTranslationUnit TU;
 } IndexData;
 
 static void free_client_data(IndexData *index_data) {
   IndexDataStringList *node = index_data->strings;
   while (node) {

 }
 
 static void printCXIndexLoc(CXIdxLoc loc, CXClientData client_data) {
   IndexData *index_data;
   CXString filename;
   const char *cname;
   CXIdxClientFile file;
   unsigned line, column;
+  const char *main_filename;
   int isMainFile;
   
   index_data = (IndexData *)client_data;
   clang_indexLoc_getFileLocation(loc, &file, 0, &line, &column, 0);
   if (line == 0) {
     printf("<invalid>");
     return;
   }
   if (!file) {
     printf("<no idxfile>");
     return;
   }
   filename = clang_getFileName((CXFile)file);
   cname = clang_getCString(filename);
-  if (strcmp(cname, index_data->main_filename) == 0)
+  main_filename = clang_getCString(index_data->main_filename);
+  if (strcmp(cname, main_filename) == 0)
     isMainFile = 1;
   else
     isMainFile = 0;
   clang_disposeString(filename);
 
   if (!isMainFile) {
     printCXIndexFile(file);
     printf(":");
if (getenv("CINDEXTEST_FAILONERROR") &&
       index_data->fail_for_error = 1;
     }
   }
 }
 
 static CXIdxClientFile index_enteredMainFile(CXClientData client_data,
                                        CXFile file, void *reserved) {
   IndexData *index_data;
-  CXString filename;
 
   index_data = (IndexData *)client_data;
   printCheck(index_data);
 
-  filename = clang_getFileName(file);
-  index_data->main_filename = clang_getCString(filename);
-  clang_disposeString(filename);
+  index_data->main_filename = clang_getFileName(file);
 
   printf("[enteredMainFile]: ");
   printCXIndexFile((CXIdxClientFile)file);
   printf("\n");
 
   return (CXIdxClientFile)file;
 }
 
if (num_args == 0) {
     fprintf(stderr, "no compiler arguments\n");
     return -1;
   }
 
   index_data.check_prefix = check_prefix;
   index_data.first_check_printed = 0;
   index_data.fail_for_error = 0;
   index_data.abort = 0;
-  index_data.main_filename = "";
+  index_data.main_filename = createCXString("");
   index_data.importedASTs = importedASTs;
   index_data.strings = NULL;
   index_data.TU = NULL;
 
   index_opts = getIndexOptions();
   result = clang_indexSourceFile(idxAction, &index_data,
                                  &IndexCB,sizeof(IndexCB), index_opts,
                                  0, args, num_args, 0, 0, 0,
                                  getDefaultParsingOptions());
   if (result != CXError_Success)
     describeLibclangFailure(result);
 
   if (index_data.fail_for_error)
     result = -1;
 
+  clang_disposeString(index_data.main_filename);
   free_client_data(&index_data);
   return result;
 }
 
 static int index_ast_file(const char *ast_file,
                           CXIndex Idx,
                           CXIndexAction idxAction,
                           ImportedASTFilesData *importedASTs,
                           const char *check_prefix) {
   CXTranslationUnit TU;
   IndexData index_data;
   unsigned index_opts;
   int result;
 
   if (!CreateTranslationUnit(Idx, ast_file, &TU))
     return -1;
 
   index_data.check_prefix = check_prefix;
   index_data.first_check_printed = 0;
   index_data.fail_for_error = 0;
   index_data.abort = 0;
-  index_data.main_filename = "";
+  index_data.main_filename = createCXString("");
   index_data.importedASTs = importedASTs;
   index_data.strings = NULL;
   index_data.TU = TU;
 
   index_opts = getIndexOptions();
   result = clang_indexTranslationUnit(idxAction, &index_data,
                                       &IndexCB,sizeof(IndexCB),
                                       index_opts, TU);
   if (index_data.fail_for_error)
     result = -1;
 
   clang_disposeTranslationUnit(TU);
+  clang_disposeString(index_data.main_filename);
   free_client_data(&index_data);
   return result;
 }
 
 static int index_file(int argc, const char **argv, int full) {
   const char *check_prefix;
   CXIndex Idx;
   CXIndexAction idxAction;
while (I != E) {
     switch (len) {
       case 8:
         if (memcmp(kind, "ObjCIvar", 8) == 0) {
           if (I + 2 >= E)
             return insufficient_usr(kind, "<ivar name> <class USR>");
           if (!isUSR(I[2]))
             return not_usr("<class USR>", I[2]);
           else {
-            CXString x;
-            x.data = (void*) I[2];
-            x.private_flags = 0;
+            CXString x = createCXString(I[2]);
             print_usr(clang_constructUSR_ObjCIvar(I[1], x));
           }
 
           I += 3;
           continue;
         }
         break;
       case 9:
         if (memcmp(kind, "ObjCClass", 9) == 0) {
           if (I + 1 >= E)
             return insufficient_usr(kind, "<class name>");
           print_usr(clang_constructUSR_ObjCClass(I[1]));
           I += 2;
           continue;
         }
         break;
       case 10:
         if (memcmp(kind, "ObjCMethod", 10) == 0) {
           if (I + 3 >= E)
             return insufficient_usr(kind, "<method selector> "
                 "[0=class method|1=instance method] <class USR>");
           if (!isUSR(I[3]))
             return not_usr("<class USR>", I[3]);
           else {
-            CXString x;
-            x.data = (void*) I[3];
-            x.private_flags = 0;
+            CXString x = createCXString(I[3]);
             print_usr(clang_constructUSR_ObjCMethod(I[1], atoi(I[2]), x));
           }
           I += 4;
           continue;
         }
         break;
       case 12:
         if (memcmp(kind, "ObjCCategory", 12) == 0) {
switch (len) {
           continue;
         }
         if (memcmp(kind, "ObjCProperty", 12) == 0) {
           if (I + 2 >= E)
             return insufficient_usr(kind, "<property name> <class USR>");
           if (!isUSR(I[2]))
             return not_usr("<class USR>", I[2]);
           else {
-            CXString x;
-            x.data = (void*) I[2];
-            x.private_flags = 0;
+            CXString x = createCXString(I[2]);
             print_usr(clang_constructUSR_ObjCProperty(I[1], x));
           }
           I += 3;
           continue;
         }
         break;
       default:
         break;

tools/libclang/CXString.h

 
 }
 
 static inline StringRef getContents(const CXUnsavedFile &UF) {
   return StringRef(UF.Contents, UF.Length);
 }
 }
 
 #endif
-

tools/libclang/CXString.cpp

 // This file defines routines for manipulating CXStrings. It should be the
 // only file that has internal knowledge of the encoding of the data in
 // CXStrings.
 //
 //===----------------------------------------------------------------------===//
 
 #include "CXString.h"
 #include "CXTranslationUnit.h"
+#include "clang-c/CXString.h"
 #include "clang-c/Index.h"
 #include "clang/Frontend/ASTUnit.h"
 #include "llvm/Support/ErrorHandling.h"
+#include <cstring>
 
-using namespace clang;
+static_assert(sizeof(CXString) <= 16, "");
 
-/// Describes the kind of underlying data in CXString.
-enum CXStringFlag {
-  /// CXString contains a 'const char *' that it doesn't own.
-  CXS_Unmanaged,
-
-  /// CXString contains a 'const char *' that it allocated with malloc().
-  CXS_Malloc,
-
-  /// CXString contains a CXStringBuf that needs to be returned to the
-  /// CXStringPool.
-  CXS_StringBuf
-};
+using namespace clang;
 
 namespace clang {
 namespace cxstring {
 
+/**
+ * This is for \b CXString 's which are created with \b CreateRef(StringRef).
+ * We'll store the info from the input \b StringRef: char ptr and size.
+ *
+ * We don't know for sure whether this is null-terminated so, when and if
+ * \b clang_getCString is called for this \b CXString, we'll allocate C string
+ * storage and copy data into the storage.  We'll memo-ize that in the
+ * \b CString member.
+ *
+ * This is refcounted; the \b Count member is initially 1.  When a \b CXString
+ * instance using this object is disposed via \b clang_disposeString, \b Count
+ * is decremented.  When this string is duplicated the \b Count increases.
+ *
+ * When \b Count finally drops to zero, the ptr at \b CString, and this object,
+ * should be freed with \b free .
+ */
+struct RefCountedCharRange {
+  const char *Data;
+  const char *CString;
+  unsigned Size;
+  unsigned Count;
+};
+
 //===----------------------------------------------------------------------===//
 // Basic generation of CXStrings.
 //===----------------------------------------------------------------------===//
 
 CXString createEmpty() {
   CXString Str;
-  Str.data = "";
-  Str.private_flags = CXS_Unmanaged;
+  Str.Contents = (const void *) "";
+  Str.Size = 0;
+  Str.IsNullTerminated = true;
+  Str.IsOwned = false;
+  Str.IsPooled = false;
   return Str;
 }
 
 CXString createNull() {
   CXString Str;
-  Str.data = nullptr;
-  Str.private_flags = CXS_Unmanaged;
+  Str.Contents = nullptr;
+  Str.Size = 0;
+  Str.IsNullTerminated = false;
+  Str.IsOwned = false;
+  Str.IsPooled = false;
   return Str;
 }
 
 CXString createRef(const char *String) {
-  if (String && String[0] == '\0')
+  if (!String) {
+    return createNull();
+  }
+  if (String[0] == '\0') {
     return createEmpty();
+  }
 
   CXString Str;
-  Str.data = String;
-  Str.private_flags = CXS_Unmanaged;
+  Str.Contents = (const void *) String;
+  Str.Size = strlen(String);
+  Str.IsNullTerminated = true;
+  Str.IsOwned = false;
+  Str.IsPooled = false;
   return Str;
 }
 
+inline static const char *copyCharRange(const char *CS, unsigned Size) {
+  char *Spelling = static_cast<char *>(malloc(Size + 1));
+  memcpy(Spelling, CS, Size);
+  Spelling[Size] = 0;
+  return Spelling;
+}
+
 CXString createDup(const char *String) {
-  if (!String)
+  if (!String) {
     return createNull();
-
-  if (String[0] == '\0')
+  }
+  if (String[0] == '\0') {
     return createEmpty();
+  }
 
   CXString Str;
-  Str.data = strdup(String);
-  Str.private_flags = CXS_Malloc;
+  Str.Size = strlen(String);
+  Str.Contents = (const void *) copyCharRange(String, Str.Size);
+  Str.IsNullTerminated = true;
+  Str.IsOwned = true;
+  Str.IsPooled = false;
   return Str;
 }
 
 CXString createRef(StringRef String) {
-  // If the string is not nul-terminated, we have to make a copy.
-
-  // FIXME: This is doing a one past end read, and should be removed! For memory
-  // we don't manage, the API string can become unterminated at any time outside
-  // our control.
-
-  if (!String.empty() && String.data()[String.size()] != 0)
-    return createDup(String);
-
-  CXString Result;
-  Result.data = String.data();
-  Result.private_flags = (unsigned) CXS_Unmanaged;
-  return Result;
+  assert (String.size() <= std::numeric_limits<unsigned>::max());
+  CXString Str;
+  Str.Size = unsigned(String.size());
+  Str.IsNullTerminated = false;
+  Str.IsOwned = false;
+  Str.IsPooled = false;
+  auto *RC = new RefCountedCharRange {
+    /* Data */ String.data(),
+    /* CString */ nullptr,
+    /* Size */ Str.Size,
+    /* Count */ 1,
+  };
+  Str.Contents = (const void *) RC;
+  return Str;
 }
 
 CXString createDup(StringRef String) {
-  CXString Result;
-  char *Spelling = static_cast<char *>(malloc(String.size() + 1));
-  memmove(Spelling, String.data(), String.size());
-  Spelling[String.size()] = 0;
-  Result.data = Spelling;
-  Result.private_flags = (unsigned) CXS_Malloc;
-  return Result;
+  CXString Str;
+  Str.Size = String.size();
+  Str.Contents = (const void *) copyCharRange(String.data(), Str.Size);
+  Str.IsNullTerminated = true;
+  Str.IsOwned = true;
+  Str.IsPooled = false;
+  return Str;
 }
 
 CXString createCXString(CXStringBuf *buf) {
   CXString Str;
-  Str.data = buf;
-  Str.private_flags = (unsigned) CXS_StringBuf;
+  Str.Contents = buf->Data.data();
+  Str.Size = buf->Data.size();
+  Str.IsNullTerminated = true;
+  Str.IsOwned = false;
+  Str.IsPooled = true;
   return Str;
 }
 
 CXStringSet *createSet(const std::vector<std::string> &Strings) {
   CXStringSet *Set = new CXStringSet;
   Set->Count = Strings.size();
   Set->Strings = new CXString[Set->Count];
   for (unsigned SI = 0, SE = Set->Count; SI < SE; ++SI)
CXStringBuf *getCXStringBuf(CXTranslationUnit TU) {
   return TU->StringPool->getCXStringBuf(TU);
 }
 
 void CXStringBuf::dispose() {
   TU->StringPool->Pool.push_back(this);
 }
 
 bool isManagedByPool(CXString str) {
-  return ((CXStringFlag) str.private_flags) == CXS_StringBuf;
+  return str.IsPooled;
 }
 
 } // end namespace cxstring
 } // end namespace clang
 
 //===----------------------------------------------------------------------===//
 // libClang public APIs.
 //===----------------------------------------------------------------------===//
 
 const char *clang_getCString(CXString string) {
-  if (string.private_flags == (unsigned) CXS_StringBuf) {
-    return static_cast<const cxstring::CXStringBuf *>(string.data)->Data.data();
-  }
-  return static_cast<const char *>(string.data);
+  const char *CString = nullptr;
+
+  // If not null-terminated, then this is a reference to a range of characters
+  // owned elsewhere.  We'll need to copy this to a new C string to ensure it
+  // is null-terminated, if we haven't done so already.
+  if (string.IsNullTerminated) {
+    CString = (const char *) string.Contents;
+  } else {
+    auto *V = const_cast<void *>(string.Contents);
+    if (V) {
+      auto *R = (clang::cxstring::RefCountedCharRange *) V;
+      if (R) {
+        if (!R->CString) {
+          // Need to make this a new C string.
+          char *NewCString = (char *) malloc(R->Size + 1);
+          memcpy(NewCString, R->Data, R->Size);
+          NewCString[R->Size] = 0;
+          R->CString = (const char *) NewCString;
+        }
+        CString = R->CString;
+      }
+    }
+  }
+
+  return CString;
+
+const char *clang_getStringData(CXString string) {
+  if (!(string.IsOwned || string.IsNullTerminated)) {
+    auto *R = (const clang::cxstring::RefCountedCharRange *) string.Contents;
+    return R ? R->Data : nullptr;
+  }
+  return (const char *) string.Contents;
+}
+
+unsigned clang_getStringSize(CXString string) {
+  return string.Size;
 }
 
 void clang_disposeString(CXString string) {
-  switch ((CXStringFlag) string.private_flags) {
-    case CXS_Unmanaged:
-      break;
-    case CXS_Malloc:
-      if (string.data)
-        free(const_cast<void *>(string.data));
-      break;
-    case CXS_StringBuf:
-      static_cast<cxstring::CXStringBuf *>(
-          const_cast<void *>(string.data))->dispose();
-      break;
+  if (string.IsOwned) {
+    // Is a C string that we own; need to free it.
+    free(const_cast<void *>(string.Contents));
+  } else {
+    if (!string.IsNullTerminated) {
+      // Neither null-terminated nor owned.  Is a char range for which we might
+      // have created a C string.
+      auto *CR = (const clang::cxstring::RefCountedCharRange *) string.Contents;
+      auto *R = const_cast<clang::cxstring::RefCountedCharRange *>(CR);
+      if (R) {
+        -- R->Count;
+        if (!R->Count) {
+          free((void *) R->CString);
+          free((void *) R);
+        }
+      }
+    }
   }
 }
 
 void clang_disposeStringSet(CXStringSet *set) {
   for (unsigned SI = 0, SE = set->Count; SI < SE; ++SI)
     clang_disposeString(set->Strings[SI]);
   delete[] set->Strings;
   delete set;
 }
-