Bug 1402247 part 6 - Fix fatal bugs. draft
authorHenri Sivonen <hsivonen@hsivonen.fi>
Thu, 01 Mar 2018 17:02:47 +0200
changeset 768032 162bcfce6cade5b58f29f0a289bc44355a5442fe
parent 768031 f41bb7a7483db4c29cda50cc7bf60b2fca037622
child 768033 b8b72e2d353ba24a5efbffdab728463533b54ed3
push id102784
push userbmo:hsivonen@hsivonen.fi
push dateThu, 15 Mar 2018 16:05:17 +0000
bugs1402247
milestone61.0a1
Bug 1402247 part 6 - Fix fatal bugs. MozReview-Commit-ID: CFHiAdrK0Lo
servo/support/gecko/nsstring/src/conversions.rs
xpcom/string/nsReadableUtils.cpp
xpcom/string/nsReadableUtils.h
xpcom/string/nsTSubstring.h
--- a/servo/support/gecko/nsstring/src/conversions.rs
+++ b/servo/support/gecko/nsstring/src/conversions.rs
@@ -20,29 +20,41 @@ fn times_three_plus_one(a: usize) -> Opt
     a.checked_mul(3)?.checked_add(1)
 }
 
 #[inline(always)]
 fn fallible_times_two(a: usize) -> Result<usize, ()> {
     a.checked_mul(2).ok_or(())
 }
 
+#[inline(always)]
+fn identity(a: usize) -> Option<usize> {
+    Some(a)
+}
+
+#[inline(always)]
+fn plus_one(a: usize) -> Option<usize> {
+    a.checked_add(1)
+}
+
 /// A conversion where the number of code units in the output is potentially
 /// smaller than the number of code units in the input.
 ///
 /// Takes the name of the method to be generated, the name of the conversion
 /// function and the type of the input slice.
 macro_rules! shrinking_conversion {
     ($name:ident,
      $convert:ident,
-     $other_ty:ty) => (
+     $other_ty:ty,
+     $math:ident) => (
         fn $name(&mut self, other: $other_ty, old_len: usize) -> Result<(), ()> {
             let written = {
+                let needed = $math(other.len()).ok_or(())?;
                 let buffer = unsafe {
-                    self.fallible_maybe_expand_capacity(old_len.checked_add(other.len()).ok_or(())?)?
+                    self.fallible_maybe_expand_capacity(old_len.checked_add(needed).ok_or(())?)?
                 };
                 $convert(other, &mut buffer[old_len..])
             };
             unsafe {
                 // TODO: Shrink buffer
                 self.fallible_set_length((old_len + written) as u32)?;
             }
             Ok(())
@@ -108,17 +120,19 @@ macro_rules! ascii_copy_avoidance {
         }
     )
 }
 
 impl nsAString {
 
     // Valid UTF-8 to UTF-16
 
-    shrinking_conversion!(fallible_append_str_impl, convert_str_to_utf16, &str);
+    // Documentation says the destination buffer needs to have
+    // as many code units as the input.
+    shrinking_conversion!(fallible_append_str_impl, convert_str_to_utf16, &str, identity);
 
     /// Convert a valid UTF-8 string into valid UTF-16 and replace the content
     /// of this string with the conversion result.
     pub fn assign_str(&mut self, other: &str) {
         self.fallible_append_str_impl(other, 0).expect("Out of memory");
     }
 
     /// Convert a valid UTF-8 string into valid UTF-16 and fallibly replace the
@@ -138,17 +152,19 @@ impl nsAString {
     /// conversion to this string.
     pub fn fallible_append_str(&mut self, other: &str) -> Result<(), ()> {
         let len = self.len();
         self.fallible_append_str_impl(other, len)
     }
 
     // Potentially-invalid UTF-8 to UTF-16
 
-    shrinking_conversion!(fallible_append_utf8_impl, convert_utf8_to_utf16, &[u8]);
+    // Documentation says the destination buffer needs to have
+    // one more code unit than the input.
+    shrinking_conversion!(fallible_append_utf8_impl, convert_utf8_to_utf16, &[u8], plus_one);
 
     /// Convert a potentially-invalid UTF-8 string into valid UTF-16
     /// (replacing invalid sequences with the REPLACEMENT CHARACTER) and
     /// replace the content of this string with the conversion result.
     pub fn assign_utf8(&mut self, other: &[u8]) {
         self.fallible_append_utf8_impl(other, 0).expect("Out of memory");
     }
 
--- a/xpcom/string/nsReadableUtils.cpp
+++ b/xpcom/string/nsReadableUtils.cpp
@@ -25,18 +25,19 @@ using mozilla::MakeSpan;
  * @return a new buffer (of the type specified by the second parameter) which you must free with |free|.
  *
  */
 template <class FromStringT, class ToCharT>
 inline
 ToCharT*
 AllocateStringCopy(const FromStringT& aSource, ToCharT*)
 {
+  // XXX can this overflow?
   return static_cast<ToCharT*>(moz_xmalloc(
-    (aSource.Length() + 1) * sizeof(ToCharT)));
+    (size_t(aSource.Length()) + 1) * sizeof(ToCharT)));
 }
 
 
 char*
 ToNewCString(const nsAString& aSource)
 {
   char* dest = AllocateStringCopy(aSource, (char*)nullptr);
   if (!dest) {
@@ -152,23 +153,35 @@ UTF8ToUnicodeBuffer(const nsACString& aS
     *aUTF16Count = converter.Length();
   }
   return aBuffer;
 }
 
 char16_t*
 UTF8ToNewUnicode(const nsACString& aSource, uint32_t* aUTF16Count)
 {
-  char16_t* dest = AllocateStringCopy(aSource, (char16_t*)nullptr);
+  // Compute length plus one as required by ConvertUTF8toUTF16Func
+  uint32_t lengthPlusOne = aSource.Length() + 1; // Can't overflow
+
+  mozilla::CheckedInt<size_t> allocLength(lengthPlusOne);
+  // Add space for zero-termination
+  allocLength += 1;
+  // We need UTF-16 units
+  allocLength *= sizeof(char16_t);
+
+  if (!allocLength.isValid()) {
+    return nullptr;
+  }
+
+  char16_t* dest = (char16_t*)moz_xmalloc(allocLength.value());
   if (!dest) {
     return nullptr;
   }
 
-  auto len = aSource.Length();
-  size_t written = ConvertUTF8toUTF16Func(aSource, MakeSpan(dest, len));
+  size_t written = ConvertUTF8toUTF16Func(aSource, MakeSpan(dest, lengthPlusOne));
   dest[written] = 0;
 
   if (aUTF16Count) {
     *aUTF16Count = written;
   }
 
   return dest;
 }
--- a/xpcom/string/nsReadableUtils.h
+++ b/xpcom/string/nsReadableUtils.h
@@ -74,17 +74,17 @@ inline size_t
 ConvertUTF16toUTF8Func(mozilla::Span<const char16_t> aSource, mozilla::Span<char> aDest)
 {
   return encoding_mem_convert_utf16_to_utf8(aSource.Elements(), aSource.Length(), aDest.Elements(), aDest.Length());
 }
 
 /**
  * Malformed byte sequences are replaced with the REPLACEMENT CHARACTER.
  *
- * The length of aDest must be not be less than the length of aSource.
+ * The length of aDest must at least one greater than the length of aSource.
  *
  * Returns the number of code units written.
  */
 inline size_t
 ConvertUTF8toUTF16Func(mozilla::Span<const char> aSource, mozilla::Span<char16_t> aDest)
 {
   return encoding_mem_convert_utf8_to_utf16(aSource.Elements(), aSource.Length(), aDest.Elements(), aDest.Length());
 }
--- a/xpcom/string/nsTSubstring.h
+++ b/xpcom/string/nsTSubstring.h
@@ -605,17 +605,17 @@ public:
    *
    * Note that unlike GetMutableData, this rounds the length up to the
    * capacity.
    */
   inline char_type* MaybeExpandCapacity(size_type* aCapacity, const fallible_t& aFallible)
   {
     // SetCapacity unshares a shared buffer even then resizing is not
     // needed.
-    if (SetCapacity(*aCapacity, aFallible)) {
+    if (!SetCapacity(*aCapacity, aFallible)) {
       return nullptr;
     }
     size_type capacity = Capacity();
     // SetCapacity doesn't stretch the logical length for us.
     this->mLength = capacity;
     *aCapacity = capacity;
     char_type* ptr = base_string_type::mData;
     // SetCapacity zero-terminated at intermediate length, not capacity.