From 1442c4ffcd03a6acab9890fecc7571f008becb73 Mon Sep 17 00:00:00 2001
From: Markus Scherer <markus.icu@gmail.com>
Date: Tue, 17 Sep 2013 21:44:35 +0000
Subject: [PATCH] ICU-9740 fix UnicodeString::getTerminatedBuffer() vs.
 truncated read-only alias

X-SVN-Rev: 34365
---
 icu4c/source/common/unicode/unistr.h    | 51 +------------------------
 icu4c/source/common/unistr.cpp          | 38 ++++++++++++++++++
 icu4c/source/test/intltest/ustrtest.cpp |  8 ++++
 3 files changed, 48 insertions(+), 49 deletions(-)

diff --git a/icu4c/source/common/unicode/unistr.h b/icu4c/source/common/unicode/unistr.h
index d7e9a2df737..248f1d5412e 100644
--- a/icu4c/source/common/unicode/unistr.h
+++ b/icu4c/source/common/unicode/unistr.h
@@ -2846,7 +2846,7 @@ public:
    * @see getBuffer()
    * @stable ICU 2.2
    */
-  inline const UChar *getTerminatedBuffer();
+  const UChar *getTerminatedBuffer();
 
   //========================================
   // Constructors
@@ -4282,48 +4282,6 @@ UnicodeString::setArray(UChar *array, int32_t len, int32_t capacity) {
   fUnion.fFields.fCapacity = capacity;
 }
 
-inline const UChar *
-UnicodeString::getTerminatedBuffer() {
-  if(!isWritable()) {
-    return 0;
-  } else {
-    UChar *array = getArrayStart();
-    int32_t len = length();
-    if(len < getCapacity() && ((fFlags&kRefCounted) == 0 || refCount() == 1)) {
-      /*
-       * kRefCounted: Do not write the NUL if the buffer is shared.
-       * That is mostly safe, except when the length of one copy was modified
-       * without copy-on-write, e.g., via truncate(newLength) or remove(void).
-       * Then the NUL would be written into the middle of another copy's string.
-       */
-      if(!(fFlags&kBufferIsReadonly)) {
-        /*
-         * We must not write to a readonly buffer, but it is known to be
-         * NUL-terminated if len<capacity.
-         * A shared, allocated buffer (refCount()>1) must not have its contents
-         * modified, but the NUL at [len] is beyond the string contents,
-         * and multiple string objects and threads writing the same NUL into the
-         * same location is harmless.
-         * In all other cases, the buffer is fully writable and it is anyway safe
-         * to write the NUL.
-         *
-         * Note: An earlier version of this code tested whether there is a NUL
-         * at [len] already, but, while safe, it generated lots of warnings from
-         * tools like valgrind and Purify.
-         */
-        array[len] = 0;
-      }
-      return array;
-    } else if(cloneArrayIfNeeded(len+1)) {
-      array = getArrayStart();
-      array[len] = 0;
-      return array;
-    } else {
-      return 0;
-    }
-  }
-}
-
 inline UnicodeString&
 UnicodeString::operator= (UChar ch)
 { return doReplace(0, length(), &ch, 0, 1); }
@@ -4456,9 +4414,7 @@ inline UnicodeString&
 UnicodeString::remove()
 {
   // remove() of a bogus string makes the string empty and non-bogus
-  // we also un-alias a read-only alias to deal with NUL-termination
-  // issues with getTerminatedBuffer()
-  if(fFlags & (kIsBogus|kBufferIsReadonly)) {
+  if(isBogus()) {
     setToEmpty();
   } else {
     fShortLength = 0;
@@ -4497,9 +4453,6 @@ UnicodeString::truncate(int32_t targetLength)
     return FALSE;
   } else if((uint32_t)targetLength < (uint32_t)length()) {
     setLength(targetLength);
-    if(fFlags&kBufferIsReadonly) {
-      fUnion.fFields.fCapacity = targetLength;  // not NUL-terminated any more
-    }
     return TRUE;
   } else {
     return FALSE;
diff --git a/icu4c/source/common/unistr.cpp b/icu4c/source/common/unistr.cpp
index b1c24769a01..5066fb4137a 100644
--- a/icu4c/source/common/unistr.cpp
+++ b/icu4c/source/common/unistr.cpp
@@ -1129,6 +1129,44 @@ UnicodeString::unBogus() {
   }
 }
 
+const UChar *
+UnicodeString::getTerminatedBuffer() {
+  if(!isWritable()) {
+    return 0;
+  }
+  UChar *array = getArrayStart();
+  int32_t len = length();
+  if(len < getCapacity()) {
+    if(fFlags & kBufferIsReadonly) {
+      // If len<capacity on a read-only alias, then array[len] is
+      // either the original NUL (if constructed with (TRUE, s, length))
+      // or one of the original string contents characters (if later truncated),
+      // therefore we can assume that array[len] is initialized memory.
+      if(array[len] == 0) {
+        return array;
+      }
+    } else if(((fFlags & kRefCounted) == 0 || refCount() == 1)) {
+      // kRefCounted: Do not write the NUL if the buffer is shared.
+      // That is mostly safe, except when the length of one copy was modified
+      // without copy-on-write, e.g., via truncate(newLength) or remove(void).
+      // Then the NUL would be written into the middle of another copy's string.
+
+      // Otherwise, the buffer is fully writable and it is anyway safe to write the NUL.
+      // Do not test if there is a NUL already because it might be uninitialized memory.
+      // (That would be safe, but tools like valgrind & Purify would complain.)
+      array[len] = 0;
+      return array;
+    }
+  }
+  if(cloneArrayIfNeeded(len+1)) {
+    array = getArrayStart();
+    array[len] = 0;
+    return array;
+  } else {
+    return NULL;
+  }
+}
+
 // setTo() analogous to the readonly-aliasing constructor with the same signature
 UnicodeString &
 UnicodeString::setTo(UBool isTerminated,
diff --git a/icu4c/source/test/intltest/ustrtest.cpp b/icu4c/source/test/intltest/ustrtest.cpp
index fb8c3457ebf..c13fce8b299 100644
--- a/icu4c/source/test/intltest/ustrtest.cpp
+++ b/icu4c/source/test/intltest/ustrtest.cpp
@@ -1174,6 +1174,14 @@ UnicodeStringTest::TestMiscellaneous()
         errln("UnicodeString(shared buffer).remove().getTerminatedBuffer() "
               "modified another copy of the string!");
     }
+
+    // ticket #9740
+    test1.setTo(TRUE, ucs, 3);
+    assertEquals("length of read-only alias", 3, test1.length());
+    test1.trim();
+    assertEquals("length of read-only alias after trim()", 2, test1.length());
+    assertEquals("length of terminated buffer of read-only alias + trim()",
+                 2, u_strlen(test1.getTerminatedBuffer()));
 }
 
 void
-- 
2.40.0