353641 - ToLowerCase assumes iterators comparable but old string ones aren't

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[Ian Jackson]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1b2) Gecko/20060601 BonEcho/2.0b2 (Ubuntu-edgy)
Build Identifier: firefox 1.5.0.7 source tarball as patched by Debian and then Ubuntu

I have been attempting to get some old versions of Gecko-based programs, including yelp 2.12.1 and epiphany 1.8.2, to build and run against Gecko from firefox 1.5.  The combination was crashing deep inside copy_string as called by the V1 string ABI version of ToLowerCase (nsUnicharUtils.cpp line 1119).

Investigation with valgrind and adding printfs showed that the problem was as follows: ToLowerCase calls BeginWriting and EndWriting on its input string, and passes these along with the ConverToLowerCase converter to copy_string.  copy_string finds the length of the string by (eventually) subtracting the data pointers in the beginning and ending iterators.  However, the old string ABI versions of BeginWriting and EndWriting seem to copy the data somehow - the two base data pointers are not the same.

I have worked around this with the patch shown below, which I'm pretty sure isn't sufficient even though it makes the symptoms go away.  Note the spurious call to EndWriting which seems to be needed to avoid a spurious double-free of the data pointer (which is copied using the default copy-assignment constructure as is approved of by the relevant header file, even though apparently the iterator contains a pointer to a dynamically allocated string).

I think the real bug is that the string is copied and a new data pointer made for the iterator, but I wasn't able to follow my way through the twisty maze of old and new string classes, vtables, etc.  Nevertheless this patch appears to make our applications work and I think I have analysed the cause of the bug correctly at least as far as I go.

Reproducible: Always

Steps to Reproduce:
1. Download firefox_1.5.dfsg+1.5.0.5-0ubuntu6.06.1.dsc and its referenced files from archive.ubuntu.com.  Remove all references to FC_ANY_METRICS.  Install the build depencencies and compile on Ubuntu breezy and install the resulting .debs.
2. Download yelp_2.12.1-0ubuntu1.dsc and its referenced files.  Install the build-dependencies, making sure to use the firefox-dev et al from step 1.  Compile it.
3. Run the resulting program; it will crash.  To find the location of the crash, run it under valgrind.  You will need to suppress a UMR in NotifyObservers, which is irrelevant, as well as the usual UMRs in ld.so and libX11.



diff -ru samual/firefox-1.5.dfsg+1.5.0.7/intl/unicharutil/util/nsUnicharUtils.cpp firefox-1.5.dfsg+1.5.0.7/intl/unicharutil/util/nsUnicharUtils.cpp
--- samual/firefox-1.5.dfsg+1.5.0.7/intl/unicharutil/util/nsUnicharUtils.cpp    2006-06-23 08:56:19.000000000 +0100
+++ firefox-1.5.dfsg+1.5.0.7/intl/unicharutil/util/nsUnicharUtils.cpp   2006-09-21 14:21:54.000000000 +0100
@@ -117,7 +117,18 @@
   {
     nsAString::iterator fromBegin, fromEnd;
     ConvertToLowerCase converter;
-    copy_string(aString.BeginWriting(fromBegin), aString.EndWriting(fromEnd), converter);
+aString.BeginWriting(fromBegin);
+fromEnd= fromBegin;
+fromEnd.advance(fromEnd.size_forward());
+//fprintf(stderr,"iwj  B %lx %lx %lx  E %lx %lx %lx\n",
+//     ((unsigned long*)&fromBegin)[0],
+//     ((unsigned long*)&fromBegin)[1],
+//     ((unsigned long*)&fromBegin)[2],
+//     ((unsigned long*)&fromEnd)[0],
+//     ((unsigned long*)&fromEnd)[1],
+//     ((unsigned long*)&fromEnd)[2]);
+    copy_string(fromBegin, fromEnd, converter);
+aString.EndWriting(fromEnd);
   }
 #endif
 
@@ -207,7 +218,18 @@
   {
     nsAString::iterator fromBegin, fromEnd;
     ConvertToUpperCase converter;
-    copy_string(aString.BeginWriting(fromBegin), aString.EndWriting(fromEnd), converter);
+aString.BeginWriting(fromBegin);
+fromEnd= fromBegin;
+fromEnd.advance(fromEnd.size_forward());
+//fprintf(stderr,"iwj  UB %lx %lx %lx  E %lx %lx %lx\n",
+//     ((unsigned long*)&fromBegin)[0],
+//     ((unsigned long*)&fromBegin)[1],
+//     ((unsigned long*)&fromBegin)[2],
+//     ((unsigned long*)&fromEnd)[0],
+//     ((unsigned long*)&fromEnd)[1],
+//     ((unsigned long*)&fromEnd)[2]);
+    copy_string(fromBegin, fromEnd, converter);
+aString.EndWriting(fromEnd);
   }
 #endif
 
Only in firefox-1.5.dfsg+1.5.0.7/intl/unicharutil/util: nsUnicharUtils.cpp~
diff -ru samual/firefox-1.5.dfsg+1.5.0.7/xpcom/string/src/nsTAString.cpp firefox-1.5.dfsg+1.5.0.7/xpcom/string/src/nsTAString.cpp
--- samual/firefox-1.5.dfsg+1.5.0.7/xpcom/string/src/nsTAString.cpp     2004-06-06 03:17:00.000000000 +0100
+++ firefox-1.5.dfsg+1.5.0.7/xpcom/string/src/nsTAString.cpp    2006-09-21 14:20:59.000000000 +0100
@@ -477,16 +477,19 @@
 nsTAString_CharT::size_type
 nsTAString_CharT::GetWritableBuffer(char_type **data)
   {
+//fprintf(stderr,"iwj GWB %p vt %p\n", this, mVTable);
     if (mVTable == obsolete_string_type::sCanonicalVTable)
       {
         substring_type* str = AsSubstring();
         str->BeginWriting(*data);
+//fprintf(stderr,"iwj GWB obs %p\n", *data);
         return str->Length();
       }
 
     obsolete_string_type::fragment_type frag;
     AsObsoleteString()->GetWritableFragment(frag, obsolete_string_type::kFirstFragment, 0);
     *data = frag.mStart;
+//fprintf(stderr,"iwj GWB new %p\n", *data);
     return (frag.mEnd - frag.mStart);
   }
 
Only in firefox-1.5.dfsg+1.5.0.7/xpcom/string/src: nsTAString.cpp~

Comment 1

[timeless]

Attachment: reporter's patch (against MOZILLA_1_8_0_BRANCH) -pU5

reporter:
1. please always use cvs diff
2. please always attach patches instead of inlining them. even patches sent to dbts are done as attachments
3. diff -pu are appreciated

Comment 2

[Ian Jackson]

DO NOT APPLY MY PATCH!

I tried to mark it as obsolete but apparently I'm not permitted to do so.  Would someone who has the relevant access please mark that diff as obsolete or otherwise arrange that it isn't considered a candidate for inclusion.

As I say in my comments, I'm pretty sure this isn't the right solution.  This needs to be looked at by someone with knowledge of the backward compatibility arrangements for the string ABIs.


> 1. please always use cvs diff

I'm afraid I don't have a CVS tree of mozilla here so that's quite inconvenient.

> 2. please always attach patches instead of inlining them. even patches sent to dbts are done as attachments

Yes, I know about that feature, but I wanted to make it clear that my patch should not be applied.  It is informative - ie it should be read by people.  Downloading it and applying it would be a mistake.

> 3. diff -pu are appreciated

Thanks for pointing out -p, which I wasn't previously aware of!

Regards,
Ian.

Comment 3

[timeless]

bug 334009 was responsible for fixing this on trunk. i presume we want to backport that and ignore the hack in this bug.

advance doesn't do what you want for multifragment strings (ignoring all other problems w/ this code).

Comment 4

[timeless]

Comment on attachment 239500 [details] [diff] [review]
reporter's patch (against MOZILLA_1_8_0_BRANCH) -pU5

the reason to post a patch is so that people can take advantage of the patch manipulation tools in bugzilla (to see more context, jump to files, ...).

your patch was wrong and doesn't remotely match style, we wouldn't have committed it anyway.

Comment 5

[Benjamin Smedberg]

Not actively working on this bug, if there is a bug here.

Comment 6

[Boris Zbarsky [:bzbarsky]]

There is a bug...  The bug, as far as I can tell, is that programs written against a frozen API started crashing when we made an internal change.  We need to fix that, no?

Comment 7

[Benjamin Smedberg]

Unicharutils was never frozen, so I'm going to say this is WONTFIX.