From: Vadim Zeitlin <vadim@wxwidgets.org>
Date: Thu, 13 Jan 2011 14:49:55 +0000 (+0000)
Subject: Fixes for parsing invalid HTML without tag ends.
X-Git-Url: https://git.saurik.com/wxWidgets.git/commitdiff_plain/3625820490496633a180ee57035f11b86c181798

Fixes for parsing invalid HTML without tag ends.

The code in wxHtmlParser supposed in many places that a '<' character must be
always followed by a '>' one and could create (and sometimes dereference)
invalid iterators if this wasn't the case resulting in asserts from MSVC debug
CRT and possibly crashes.

Fix this by ensuring that only valid iterators are used and add a trivial unit
test for wxHtmlParser which checks that it can parse invalid HTML without
crashing.

Closes #12869.

git-svn-id: https://svn.wxwidgets.org/svn/wx/wxWidgets/trunk@66678 c3d73ce0-8a6f-49c7-b76d-6d57e0e08775
---

diff --git a/src/html/htmlpars.cpp b/src/html/htmlpars.cpp
index 38c0680c23..ed99a443b6 100644
--- a/src/html/htmlpars.cpp
+++ b/src/html/htmlpars.cpp
@@ -227,7 +227,7 @@ void wxHtmlParser::CreateDOMSubTree(wxHtmlTag *cur,
             else
             {
                 while (i < end_pos && *i != wxT('>')) ++i;
-                textBeginning = i+1;
+                textBeginning = i < end_pos ? i+1 : i;
             }
         }
         else ++i;
@@ -958,7 +958,7 @@ wxHtmlParser::SkipCommentTag(wxString::const_iterator& start,
     wxString::const_iterator p = start;
 
     // comments begin with "<!--" in HTML 4.0
-    if ( p > end - 3 || *++p != '!' || *++p != '-' || *++p != '-' )
+    if ( end - start < 4 || *++p != '!' || *++p != '-' || *++p != '-' )
     {
         // not a comment at all
         return false;
diff --git a/src/html/htmltag.cpp b/src/html/htmltag.cpp
index b2af5eff7f..fdc71fe171 100644
--- a/src/html/htmltag.cpp
+++ b/src/html/htmltag.cpp
@@ -94,12 +94,10 @@ wxHtmlTagsCache::wxHtmlTagsCache(const wxString& source)
         if ( wxHtmlParser::SkipCommentTag(pos, end) )
             continue;
 
-        size_t tg = Cache().size();
-        Cache().push_back(wxHtmlCacheItem());
-
+        // Remember the starting tag position.
         wxString::const_iterator stpos = pos++;
-        Cache()[tg].Key = stpos;
 
+        // And look for the ending one.
         int i;
         for ( i = 0;
               pos < end && i < (int)WXSIZEOF(tagBuffer) - 1 &&
@@ -110,12 +108,26 @@ wxHtmlTagsCache::wxHtmlTagsCache(const wxString& source)
         }
         tagBuffer[i] = wxT('\0');
 
-        Cache()[tg].Name = new wxChar[i+1];
-        memcpy(Cache()[tg].Name, tagBuffer, (i+1)*sizeof(wxChar));
-
         while (pos < end && *pos != wxT('>'))
             ++pos;
 
+        if ( pos == end )
+        {
+            // We didn't find a closing bracket, this is not a valid tag after
+            // all. Notice that we need to roll back pos to avoid creating an
+            // invalid iterator when "++pos" is done in the loop statement.
+            --pos;
+
+            continue;
+        }
+
+        // We have a valid tag, add it to the cache.
+        size_t tg = Cache().size();
+        Cache().push_back(wxHtmlCacheItem());
+        Cache()[tg].Key = stpos;
+        Cache()[tg].Name = new wxChar[i+1];
+        memcpy(Cache()[tg].Name, tagBuffer, (i+1)*sizeof(wxChar));
+
         if ((stpos+1) < end && *(stpos+1) == wxT('/')) // ending tag:
         {
             Cache()[tg].type = wxHtmlCacheItem::Type_EndingTag;
@@ -223,7 +235,12 @@ void wxHtmlTagsCache::QueryTag(const wxString::const_iterator& at,
                                bool *hasEnding)
 {
     if (Cache().empty())
+    {
+        *end1 =
+        *end2 = inputEnd;
+        *hasEnding = true;
         return;
+    }
 
     if (Cache()[m_CachePos].Key != at)
     {
diff --git a/tests/Makefile.in b/tests/Makefile.in
index 02f665d2f1..995b5c84b3 100644
--- a/tests/Makefile.in
+++ b/tests/Makefile.in
@@ -202,6 +202,7 @@ TEST_GUI_OBJECTS =  \
 	test_gui_fonttest.o \
 	test_gui_image.o \
 	test_gui_rawbmp.o \
+	test_gui_htmlparser.o \
 	test_gui_htmlwindow.o \
 	test_gui_accelentry.o \
 	test_gui_menu.o \
@@ -831,6 +832,9 @@ test_gui_image.o: $(srcdir)/image/image.cpp $(TEST_GUI_ODEP)
 test_gui_rawbmp.o: $(srcdir)/image/rawbmp.cpp $(TEST_GUI_ODEP)
 	$(CXXC) -c -o $@ $(TEST_GUI_CXXFLAGS) $(srcdir)/image/rawbmp.cpp
 
+test_gui_htmlparser.o: $(srcdir)/html/htmlparser.cpp $(TEST_GUI_ODEP)
+	$(CXXC) -c -o $@ $(TEST_GUI_CXXFLAGS) $(srcdir)/html/htmlparser.cpp
+
 test_gui_htmlwindow.o: $(srcdir)/html/htmlwindow.cpp $(TEST_GUI_ODEP)
 	$(CXXC) -c -o $@ $(TEST_GUI_CXXFLAGS) $(srcdir)/html/htmlwindow.cpp
 
diff --git a/tests/html/htmlparser.cpp b/tests/html/htmlparser.cpp
new file mode 100644
index 0000000000..14a184243f
--- /dev/null
+++ b/tests/html/htmlparser.cpp
@@ -0,0 +1,75 @@
+///////////////////////////////////////////////////////////////////////////////
+// Name:        tests/html/htmlparser.cpp
+// Purpose:     wxHtmlParser tests
+// Author:      Vadim Zeitlin
+// Created:     2011-01-13
+// RCS-ID:      $Id$
+// Copyright:   (c) 2011 Vadim Zeitlin <vadim@wxwidgets.org>
+///////////////////////////////////////////////////////////////////////////////
+
+// ----------------------------------------------------------------------------
+// headers
+// ----------------------------------------------------------------------------
+
+#include "testprec.h"
+
+#if wxUSE_HTML
+
+#ifdef __BORLANDC__
+    #pragma hdrstop
+#endif
+
+#ifndef WX_PRECOMP
+#endif // WX_PRECOMP
+
+#include "wx/html/htmlpars.h"
+
+// ----------------------------------------------------------------------------
+// test class
+// ----------------------------------------------------------------------------
+
+class HtmlParserTestCase : public CppUnit::TestCase
+{
+public:
+    HtmlParserTestCase() { }
+
+private:
+    CPPUNIT_TEST_SUITE( HtmlParserTestCase );
+        CPPUNIT_TEST( Invalid );
+    CPPUNIT_TEST_SUITE_END();
+
+    void Invalid();
+
+    wxDECLARE_NO_COPY_CLASS(HtmlParserTestCase);
+};
+
+// register in the unnamed registry so that these tests are run by default
+CPPUNIT_TEST_SUITE_REGISTRATION( HtmlParserTestCase );
+
+// also include in it's own registry so that these tests can be run alone
+CPPUNIT_TEST_SUITE_NAMED_REGISTRATION( HtmlParserTestCase, "HtmlParserTestCase" );
+
+// ----------------------------------------------------------------------------
+// tests themselves
+// ----------------------------------------------------------------------------
+
+// Test that parsing invalid HTML simply fails but doesn't crash for example.
+void HtmlParserTestCase::Invalid()
+{
+    class NullParser : public wxHtmlParser
+    {
+    public:
+        virtual wxObject *GetProduct() { return NULL; }
+
+    protected:
+        virtual void AddText(const wxString& WXUNUSED(txt)) { }
+    };
+
+    NullParser p;
+    p.Parse("<");
+    p.Parse("<foo");
+    p.Parse("<!--");
+    p.Parse("<!---");
+}
+
+#endif //wxUSE_HTML
diff --git a/tests/makefile.bcc b/tests/makefile.bcc
index 2a366f22d8..dea2043537 100644
--- a/tests/makefile.bcc
+++ b/tests/makefile.bcc
@@ -187,6 +187,7 @@ TEST_GUI_OBJECTS =  \
 	$(OBJS)\test_gui_fonttest.obj \
 	$(OBJS)\test_gui_image.obj \
 	$(OBJS)\test_gui_rawbmp.obj \
+	$(OBJS)\test_gui_htmlparser.obj \
 	$(OBJS)\test_gui_htmlwindow.obj \
 	$(OBJS)\test_gui_accelentry.obj \
 	$(OBJS)\test_gui_menu.obj \
@@ -877,6 +878,9 @@ $(OBJS)\test_gui_image.obj: .\image\image.cpp
 $(OBJS)\test_gui_rawbmp.obj: .\image\rawbmp.cpp
 	$(CXX) -q -c -P -o$@ $(TEST_GUI_CXXFLAGS) .\image\rawbmp.cpp
 
+$(OBJS)\test_gui_htmlparser.obj: .\html\htmlparser.cpp
+	$(CXX) -q -c -P -o$@ $(TEST_GUI_CXXFLAGS) .\html\htmlparser.cpp
+
 $(OBJS)\test_gui_htmlwindow.obj: .\html\htmlwindow.cpp
 	$(CXX) -q -c -P -o$@ $(TEST_GUI_CXXFLAGS) .\html\htmlwindow.cpp
 
diff --git a/tests/makefile.gcc b/tests/makefile.gcc
index ee9e554aa6..9c4aa0e01b 100644
--- a/tests/makefile.gcc
+++ b/tests/makefile.gcc
@@ -180,6 +180,7 @@ TEST_GUI_OBJECTS =  \
 	$(OBJS)\test_gui_fonttest.o \
 	$(OBJS)\test_gui_image.o \
 	$(OBJS)\test_gui_rawbmp.o \
+	$(OBJS)\test_gui_htmlparser.o \
 	$(OBJS)\test_gui_htmlwindow.o \
 	$(OBJS)\test_gui_accelentry.o \
 	$(OBJS)\test_gui_menu.o \
@@ -858,6 +859,9 @@ $(OBJS)\test_gui_image.o: ./image/image.cpp
 $(OBJS)\test_gui_rawbmp.o: ./image/rawbmp.cpp
 	$(CXX) -c -o $@ $(TEST_GUI_CXXFLAGS) $(CPPDEPS) $<
 
+$(OBJS)\test_gui_htmlparser.o: ./html/htmlparser.cpp
+	$(CXX) -c -o $@ $(TEST_GUI_CXXFLAGS) $(CPPDEPS) $<
+
 $(OBJS)\test_gui_htmlwindow.o: ./html/htmlwindow.cpp
 	$(CXX) -c -o $@ $(TEST_GUI_CXXFLAGS) $(CPPDEPS) $<
 
diff --git a/tests/makefile.vc b/tests/makefile.vc
index 85db68d10c..88280b900a 100644
--- a/tests/makefile.vc
+++ b/tests/makefile.vc
@@ -182,6 +182,7 @@ TEST_GUI_OBJECTS =  \
 	$(OBJS)\test_gui_fonttest.obj \
 	$(OBJS)\test_gui_image.obj \
 	$(OBJS)\test_gui_rawbmp.obj \
+	$(OBJS)\test_gui_htmlparser.obj \
 	$(OBJS)\test_gui_htmlwindow.obj \
 	$(OBJS)\test_gui_accelentry.obj \
 	$(OBJS)\test_gui_menu.obj \
@@ -1003,6 +1004,9 @@ $(OBJS)\test_gui_image.obj: .\image\image.cpp
 $(OBJS)\test_gui_rawbmp.obj: .\image\rawbmp.cpp
 	$(CXX) /c /nologo /TP /Fo$@ $(TEST_GUI_CXXFLAGS) .\image\rawbmp.cpp
 
+$(OBJS)\test_gui_htmlparser.obj: .\html\htmlparser.cpp
+	$(CXX) /c /nologo /TP /Fo$@ $(TEST_GUI_CXXFLAGS) .\html\htmlparser.cpp
+
 $(OBJS)\test_gui_htmlwindow.obj: .\html\htmlwindow.cpp
 	$(CXX) /c /nologo /TP /Fo$@ $(TEST_GUI_CXXFLAGS) .\html\htmlwindow.cpp
 
diff --git a/tests/makefile.wat b/tests/makefile.wat
index a9f8122117..90dff37c9a 100644
--- a/tests/makefile.wat
+++ b/tests/makefile.wat
@@ -422,6 +422,7 @@ TEST_GUI_OBJECTS =  &
 	$(OBJS)\test_gui_fonttest.obj &
 	$(OBJS)\test_gui_image.obj &
 	$(OBJS)\test_gui_rawbmp.obj &
+	$(OBJS)\test_gui_htmlparser.obj &
 	$(OBJS)\test_gui_htmlwindow.obj &
 	$(OBJS)\test_gui_accelentry.obj &
 	$(OBJS)\test_gui_menu.obj &
@@ -916,6 +917,9 @@ $(OBJS)\test_gui_image.obj :  .AUTODEPEND .\image\image.cpp
 $(OBJS)\test_gui_rawbmp.obj :  .AUTODEPEND .\image\rawbmp.cpp
 	$(CXX) -bt=nt -zq -fo=$^@ $(TEST_GUI_CXXFLAGS) $<
 
+$(OBJS)\test_gui_htmlparser.obj :  .AUTODEPEND .\html\htmlparser.cpp
+	$(CXX) -bt=nt -zq -fo=$^@ $(TEST_GUI_CXXFLAGS) $<
+
 $(OBJS)\test_gui_htmlwindow.obj :  .AUTODEPEND .\html\htmlwindow.cpp
 	$(CXX) -bt=nt -zq -fo=$^@ $(TEST_GUI_CXXFLAGS) $<
 
diff --git a/tests/test.bkl b/tests/test.bkl
index 42aaee6d14..9afe6d54d8 100644
--- a/tests/test.bkl
+++ b/tests/test.bkl
@@ -183,6 +183,7 @@
             font/fonttest.cpp
             image/image.cpp
             image/rawbmp.cpp
+            html/htmlparser.cpp
             html/htmlwindow.cpp
             menu/accelentry.cpp
             menu/menu.cpp
diff --git a/tests/test_test_gui.dsp b/tests/test_test_gui.dsp
index d130cb8647..2ed004defb 100644
--- a/tests/test_test_gui.dsp
+++ b/tests/test_test_gui.dsp
@@ -345,6 +345,10 @@ SOURCE=.\controls\htmllboxtest.cpp
 # End Source File
 # Begin Source File
 
+SOURCE=.\html\htmlparser.cpp
+# End Source File
+# Begin Source File
+
 SOURCE=.\html\htmlwindow.cpp
 # End Source File
 # Begin Source File
diff --git a/tests/test_vc7_test_gui.vcproj b/tests/test_vc7_test_gui.vcproj
index 0172d3a962..377614d36f 100644
--- a/tests/test_vc7_test_gui.vcproj
+++ b/tests/test_vc7_test_gui.vcproj
@@ -694,6 +694,9 @@
 			<File
 				RelativePath=".\controls\htmllboxtest.cpp">
 			</File>
+			<File
+				RelativePath=".\html\htmlparser.cpp">
+			</File>
 			<File
 				RelativePath=".\html\htmlwindow.cpp">
 			</File>
diff --git a/tests/test_vc8_test_gui.vcproj b/tests/test_vc8_test_gui.vcproj
index f300941b72..2783805612 100644
--- a/tests/test_vc8_test_gui.vcproj
+++ b/tests/test_vc8_test_gui.vcproj
@@ -999,6 +999,10 @@
 				RelativePath=".\controls\htmllboxtest.cpp"
 				>
 			</File>
+			<File
+				RelativePath=".\html\htmlparser.cpp"
+				>
+			</File>
 			<File
 				RelativePath=".\html\htmlwindow.cpp"
 				>
diff --git a/tests/test_vc9_test_gui.vcproj b/tests/test_vc9_test_gui.vcproj
index 5063d1dca8..958ea5d401 100644
--- a/tests/test_vc9_test_gui.vcproj
+++ b/tests/test_vc9_test_gui.vcproj
@@ -971,6 +971,10 @@
 				RelativePath=".\controls\htmllboxtest.cpp"
 				>
 			</File>
+			<File
+				RelativePath=".\html\htmlparser.cpp"
+				>
+			</File>
 			<File
 				RelativePath=".\html\htmlwindow.cpp"
 				>