From 0b77e1fa893897c837005211ca2e1ead799c5a2a Mon Sep 17 00:00:00 2001 From: Jonas Sicking Date: Wed, 18 Nov 2009 17:14:29 -0800 Subject: [PATCH] Bug 526500: Fix bug in OOM handing for nsTSubstring::SetLength. Also change Capacity to return 0 rather than -1 for immutable strings to avoid similar surprises in the future. Finally Make SetCapacity return a boolean indicating success/failure to make it easier to check for oom handling. r=bsmedberg sr=dbaron --- xpcom/string/public/nsTSubstring.h | 13 ++++++++-- xpcom/string/src/nsPrintfCString.cpp | 3 +-- xpcom/string/src/nsTSubstring.cpp | 38 +++++++++++----------------- 3 files changed, 27 insertions(+), 27 deletions(-) diff --git a/xpcom/string/public/nsTSubstring.h b/xpcom/string/public/nsTSubstring.h index 652bd184bd8d..87c17a9d96c3 100644 --- a/xpcom/string/public/nsTSubstring.h +++ b/xpcom/string/public/nsTSubstring.h @@ -452,7 +452,15 @@ class nsTSubstring_CharT * buffer sizing */ - NS_COM void NS_FASTCALL SetCapacity( size_type newCapacity ); + /** + * Attempts to set the capacity to the given size, without affecting + * the length of the string. Also ensures that the buffer is mutable. + * + * @returns PR_TRUE on success + * PR_FALSE on out-of-memory, or if requesting a size bigger + * than a string can hold (2^31 chars). + */ + NS_COM PRBool NS_FASTCALL SetCapacity( size_type newCapacity ); NS_COM void NS_FASTCALL SetLength( size_type newLength ); @@ -616,7 +624,8 @@ class nsTSubstring_CharT * returns the number of writable storage units starting at mData. * the value does not include space for the null-terminator character. * - * NOTE: this function returns size_type(-1) if mData is immutable. + * NOTE: this function returns 0 if mData is immutable (or the buffer + * is 0-sized). */ size_type NS_FASTCALL Capacity() const; diff --git a/xpcom/string/src/nsPrintfCString.cpp b/xpcom/string/src/nsPrintfCString.cpp index 8bb06da674aa..bda7faeb4969 100644 --- a/xpcom/string/src/nsPrintfCString.cpp +++ b/xpcom/string/src/nsPrintfCString.cpp @@ -70,8 +70,7 @@ nsPrintfCString::nsPrintfCString( size_type n, const char_type* format, ... ) size_type logical_capacity = kLocalBufferSize; if ( n > logical_capacity ) { - SetCapacity(n); - if (Capacity() < n) + if (!SetCapacity(n)) return; // out of memory !! logical_capacity = n; } diff --git a/xpcom/string/src/nsTSubstring.cpp b/xpcom/string/src/nsTSubstring.cpp index 4b2649ff57d5..a2def981b29a 100644 --- a/xpcom/string/src/nsTSubstring.cpp +++ b/xpcom/string/src/nsTSubstring.cpp @@ -101,17 +101,17 @@ nsTSubstring_CharT::MutatePrep( size_type capacity, char_type** oldData, PRUint3 // able to allocate it. Just bail out in cases like that. We don't want // to be allocating 2GB+ strings anyway. if (capacity > size_type(-1)/2) { - // Also assert for |capacity| equal to |size_type(-1)|, since we use that value to - // flag immutability. + // Also assert for |capacity| equal to |size_type(-1)|, since we used to + // use that value to flag immutability. NS_ASSERTION(capacity != size_type(-1), "Bogus capacity"); return PR_FALSE; } - // |curCapacity == size_type(-1)| means that the buffer is immutable, so we - // need to allocate a new buffer. we cannot use the existing buffer even + // |curCapacity == 0| means that the buffer is immutable or 0-sized, so we + // need to allocate a new buffer. We cannot use the existing buffer even // though it might be large enough. - if (curCapacity != size_type(-1)) + if (curCapacity != 0) { if (capacity <= curCapacity) { mFlags &= ~F_VOIDED; // mutation clears voided flag @@ -272,7 +272,7 @@ nsTSubstring_CharT::ReplacePrep( index_type cutStart, size_type cutLen, size_typ nsTSubstring_CharT::size_type nsTSubstring_CharT::Capacity() const { - // return size_type(-1) to indicate an immutable buffer + // return 0 to indicate an immutable or 0-sized buffer size_type capacity; if (mFlags & F_SHARED) @@ -280,9 +280,10 @@ nsTSubstring_CharT::Capacity() const // if the string is readonly, then we pretend that it has no capacity. nsStringBuffer* hdr = nsStringBuffer::FromData(mData); if (hdr->IsReadonly()) - capacity = size_type(-1); - else + capacity = 0; + else { capacity = (hdr->StorageSize() / sizeof(char_type)) - 1; + } } else if (mFlags & F_FIXED) { @@ -298,7 +299,7 @@ nsTSubstring_CharT::Capacity() const } else { - capacity = size_type(-1); + capacity = 0; } return capacity; @@ -554,7 +555,7 @@ nsTSubstring_CharT::Replace( index_type cutStart, size_type cutLength, const sub tuple.WriteTo(mData + cutStart, length); } -void +PRBool nsTSubstring_CharT::SetCapacity( size_type capacity ) { // capacity does not include room for the terminating null char @@ -572,7 +573,7 @@ nsTSubstring_CharT::SetCapacity( size_type capacity ) char_type* oldData; PRUint32 oldFlags; if (!MutatePrep(capacity, &oldData, &oldFlags)) - return; // out-of-memory + return PR_FALSE; // out-of-memory // compute new string length size_type newLen = NS_MIN(mLength, capacity); @@ -594,23 +595,14 @@ nsTSubstring_CharT::SetCapacity( size_type capacity ) // for backwards compat with the old string implementation. mData[capacity] = char_type(0); } + + return PR_TRUE; } void nsTSubstring_CharT::SetLength( size_type length ) { - if (mLength == length) { - mFlags &= ~F_VOIDED; // mutation clears voided flag - return; - } - - SetCapacity(length); - - // XXX(darin): SetCapacity may fail, but it doesn't give us a way to find - // out. We should improve that. For now we just verify that the capacity - // changed as expected as a means of error checking. - - if (Capacity() >= length) + if (SetCapacity(length)) mLength = length; }