flaght / google-url

Automatically exported from code.google.com/p/google-url
Other
0 stars 0 forks source link

Crash in url_util::InitStandardSchemes(), which is NOT thread safe... #17

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
While trying to use the GURL class in a multiprocess and multithreaded 
environment, I got the crash below because a new process had two threads trying 
to construct a GURL at the exact same 
time which caused a double initialization of the standard_schemes global 
variable.

Crashing thread:
    my.dll!operator delete(void * pUserData=0x041fe170)  Line 52 + 0x51 bytes   C++
    my.dll!std::allocator<char const *>::deallocate(const char * * _Ptr=0x041fe170, unsigned int __formal=1)  Line 140 + 0x9 bytes  C++
    my.dll!std::vector<char const *,std::allocator<char const *> >::_Insert_n(std::_Vector_const_iterator<char const *,std::allocator<char const *> > _Where=0xdddddddd <Bad Ptr>, 
unsigned int _Count=2, const char * const & _Val=0x64e9ff40)  Line 1202 C++
    my.dll!std::vector<char const *,std::allocator<char const *> >::insert(std::_Vector_const_iterator<char const *,std::allocator<char const *> > _Where=0xdddddddd <Bad Ptr>, const 
char * const & _Val=0x64e9ff40)  Line 878   C++
    my.dll!std::vector<char const *,std::allocator<char const *> >::push_back(const char * const & _Val=0x64e9ff40)  Line 823 + 0x27 bytes  C++
>   my.dll!url_util::`anonymous namespace'::InitStandardSchemes()  Line 82 + 0x16 
bytes   C++
    my.dll!url_util::`anonymous namespace'::IsStandardScheme<wchar_t>(const wchar_t * spec=0x042009b0, const url_parse::Component & scheme={...})  Line 107 C++
    my.dll!url_util::`anonymous namespace'::DoIsStandard<wchar_t>(const wchar_t * spec=0x042009b0, int spec_len=10, const url_parse::Component & scheme={...})  Line 133 + 0x28 bytes   
C++
    my.dll!url_util::IsStandard(const wchar_t * spec=0x042009b0, int spec_len=10, const url_parse::Component & scheme={...})  Line 345 + 0x11 bytes C++
    my.dll!url_util::`anonymous namespace'::DoCanonicalize<wchar_t>(const wchar_t * in_spec=0x042009b0, int in_spec_len=10, url_canon::CharsetConverter * charset_converter=0x00000000, 
url_canon::CanonOutputT<char> * output=0x0365cd08, url_parse::Parsed * 
output_parsed=0x0365cfe4)  Line 200 + 0x1a bytes    C++
    my.dll!url_util::Canonicalize(const wchar_t * spec=0x042009b0, int spec_len=10, url_canon::CharsetConverter * charset_converter=0x00000000, url_canon::CanonOutputT<char> * 
output=0x0365cd08, url_parse::Parsed * output_parsed=0x0365cfe4)  Line 377 + 
0x19 bytes  C++
    my.dll!`anonymous namespace'::InitCanonical<std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > >(const 
std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > & 
input_spec="about:Tabs", 
std::basic_string<char,std::char_traits<char>,std::allocator<char> > * 
canonical="", url_parse::Parsed * parsed=0x0365cfe4)  Line 59 + 0x21 bytes  C++
    my.dll!GURL::GURL(const std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > & url_string="about:Tabs")  Line 121 + 0x14 bytes C++

Other thread also initializing a GURL for the first time.
    ntdll.dll!_RtlEnterCriticalSection@4()  + 0x1a556 bytes 
    my.dll!_lock(int locknum=4)  Line 349   C
    my.dll!_heap_alloc_dbg_impl(unsigned int nSize=12, int nBlockUse=1, const char * szFileName=0x00000000, int nLine=0, int * errno_tmp=0x03e6c698)  Line 370 + 0x7 bytes  C++
    my.dll!_nh_malloc_dbg_impl(unsigned int nSize=12, int nhFlag=0, int nBlockUse=1, const char * szFileName=0x00000000, int nLine=0, int * errno_tmp=0x03e6c698)  Line 239 + 0x19 bytes    
C++
    my.dll!_nh_malloc_dbg(unsigned int nSize=12, int nhFlag=0, int nBlockUse=1, const char * szFileName=0x00000000, int nLine=0)  Line 296 + 0x1d bytes C++
    my.dll!malloc(unsigned int nSize=12)  Line 56 + 0x15 bytes  C++
    my.dll!operator new(unsigned int size=12)  Line 59 + 0x9 bytes  C++
    my.dll!std::_Allocate<char const *>(unsigned int _Count=3, const char * * __formal=0x00000000)  Line 43 + 0xc bytes C++
    my.dll!std::allocator<char const *>::allocate(unsigned int _Count=3)  Line 145 + 0xb bytes  C++
    my.dll!std::vector<char const *,std::allocator<char const *> >::_Insert_n(std::_Vector_const_iterator<char const *,std::allocator<char const *> > _Where=0xfdfdfdfd <Bad Ptr>, 
unsigned int _Count=1, const char * const & _Val=0x64e9ff28)  Line 1173 + 0xf 
bytes   C++
    my.dll!std::vector<char const *,std::allocator<char const *> >::insert(std::_Vector_const_iterator<char const *,std::allocator<char const *> > _Where=0xfdfdfdfd <Bad Ptr>, const 
char * const & _Val=0x64e9ff28)  Line 878   C++
    my.dll!std::vector<char const *,std::allocator<char const *> >::push_back(const char * const & _Val=0x64e9ff28)  Line 823 + 0x27 bytes  C++
>   my.dll!url_util::`anonymous namespace'::InitStandardSchemes()  Line 82 + 0x16 
bytes   C++
    my.dll!url_util::`anonymous namespace'::IsStandardScheme<wchar_t>(const wchar_t * spec=0x04201590, const url_parse::Component & scheme={...})  Line 107 C++
    my.dll!url_util::`anonymous namespace'::DoIsStandard<wchar_t>(const wchar_t * spec=0x04201590, int spec_len=10, const url_parse::Component & scheme={...})  Line 133 + 0x28 bytes   
C++
    my.dll!url_util::IsStandard(const wchar_t * spec=0x04201590, int spec_len=10, const url_parse::Component & scheme={...})  Line 345 + 0x11 bytes C++
    my.dll!url_util::`anonymous namespace'::DoCanonicalize<wchar_t>(const wchar_t * in_spec=0x04201590, int in_spec_len=10, url_canon::CharsetConverter * charset_converter=0x00000000, 
url_canon::CanonOutputT<char> * output=0x03e6d100, url_parse::Parsed * 
output_parsed=0x03e6d3dc)  Line 200 + 0x1a bytes    C++
    my.dll!url_util::Canonicalize(const wchar_t * spec=0x04201590, int spec_len=10, url_canon::CharsetConverter * charset_converter=0x00000000, url_canon::CanonOutputT<char> * 
output=0x03e6d100, url_parse::Parsed * output_parsed=0x03e6d3dc)  Line 377 + 
0x19 bytes  C++
    my.dll!`anonymous namespace'::InitCanonical<std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > >(const 
std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > & 
input_spec="about:Tabs", 
std::basic_string<char,std::char_traits<char>,std::allocator<char> > * 
canonical="", url_parse::Parsed * parsed=0x03e6d3dc)  Line 59 + 0x21 bytes  C++
    my.dll!GURL::GURL(const std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > & url_string="about:Tabs")  Line 121 + 0x14 bytes C++

Original issue reported on code.google.com by mad@chromium.org on 22 Feb 2010 at 8:36

GoogleCodeExporter commented 9 years ago
Is it not possible to initialize this during startup? I'm working on a patch to 
make
Chrome do this explicitly: http://codereview.chromium.org/579004
since we now need to add a set of our own schemes. This approach should be what 
you
need, if the patch itself doesn't fix your problem.

Original comment by bre...@gmail.com on 22 Feb 2010 at 9:11

GoogleCodeExporter commented 9 years ago
Unless I'm missing something, you seem to force the initialization by adding 
schemes. 
What if I don't need new schemes, how do I initialize the GURL module?

Would be nice to have an init and a term entry/exit points. It would also be 
useful to 
allow clients to destroy the global allocation, because in a plugin world, this 
leak 
could accumulate annoyingly every time the plugin DLL is loaded and freed, 
which could 
happen frequently in some cases...

Original comment by mad@chromium.org on 22 Feb 2010 at 10:43

GoogleCodeExporter commented 9 years ago
I see, if you want to add such a function to url_util.h, feel free to submit a 
patch.

If your application uses Chrome code (I have no idea), I'm guessing my new 
patch will
work, since the Chrome-specific schemes need to be added. for you.

Original comment by bre...@gmail.com on 23 Feb 2010 at 12:06

GoogleCodeExporter commented 9 years ago
How about this:

Index: src/url_util.cc
===================================================================
--- src/url_util.cc   (revision 122)
+++ src/url_util.cc   (working copy)
@@ -359,6 +359,15 @@
   return DoFindAndCompareScheme(str, str_len, compare, found_scheme);
 }

+void InitializeGoogleUrl() {
+  InitStandardSchemes();
+}
+
+void TerminateGoogleUrl() {
+  if (standard_schemes) {
+    delete standard_schemes;
+    standard_schemes = NULL;
+  }
+}
+
 bool Canonicalize(const char* spec,
                   int spec_len,
                   url_canon::CharsetConverter* charset_converter,
Index: D:/src/ceee/src/googleurl/src/url_util.h
===================================================================
--- src/url_util.h    (revision 122)
+++ src/url_util.h    (working copy)
@@ -79,6 +79,23 @@

 // URL library wrappers -------------------------------------------------------

+// Initializes the URL library to make sure there is no race in initializing
+// internal components like standard schemes. It is OK to call this more than
+// once, subsequent calls will simply "noop", unless TerminateGoogleUrl() was
+// called in the mean time. This will also be a "noop" if other calls to the
+// library have forced an initialization beforehand.
+// Be aware that there is no thread safety in this code so you should make sure
+// to call this before any other thread interacts with the URL library.
+void InitializeGoogleUrl();
+
+// Terminate the URL library so that there are no leaked internal components.
+// Note that once this is called, the URL library can still be used, it will
+// simply re-initialize itself. It is OK to call this more than once, or at a
+// point where the URL library has not been initialized yet.
+// Be aware that there is no thread safety in this code so you should make sure
+// to call this after any other thread interacts with the URL library.
+void TerminateGoogleUrl();
+
 // Parses the given spec according to the extracted scheme type. Normal users
 // should use the URL object, although this may be useful if performance is
 // critical and you don't want to do the heap allocation for the std::string.

Original comment by mad@chromium.org on 23 Feb 2010 at 4:15

GoogleCodeExporter commented 9 years ago
Chrome Frame has the same issue, crashes due to the initialization race, and 
the leak 
is bad news for a plugin. See [http://code.google.com/p/chromium/issues/detail?
id=38484] 

Original comment by si...@google.com on 24 Mar 2010 at 9:27

GoogleCodeExporter commented 9 years ago
I checked in the fix in r126. I renamed the functions Initialize and Shutdown 
since 
they were already in the url_util namespace. I did not pull it into Chrome, if 
you want 
that you should submit a DEPS change yourself.

Original comment by bre...@gmail.com on 31 Mar 2010 at 4:50