34168 - can crash parser with following dtd entry

Status: VERIFIED FIXED

(Core :: DOM: HTML Parser, defect, P1)

Description

[Syd Logan]

The following crashes <!ENTITY  editAwayMessageSpecial3.label " %%d = Current
date">

If you take away the %%, and replace with %, then the dtd is found to be
invalid. If you take away the % signs altogether, it parses just fine.

The bug is we shouldn't crash. If % signs should be recognized, perhaps that is
another bug.

Comment 1

[Blake Ross]

Attachment: testcase: may crash browser upon loading

Comment 2

[rickg]

Good catch, syd. Thanks.

Comment 3

[rickg]

On second thought, I can't reproduce this. Syd, can you verify the testcase?

Comment 4

[leger]

Adding crash keyword.

Comment 5

[Jan Carpenter]

Can't recreate this on any platform
2000-05-02-09-M16 WinNT
2000-05-01-11-M16 Linux
2000-05-01-11-M16 Mac

Comment 6

[Blake Ross]

is this fixed?

Comment 7

[Andreas Franke (gone)]

Attachment: testcase: crashing xml file

Comment 8

[Andreas Franke (gone)]

Attachment: full stack trace, PC/Linux, build 2000052109

Comment 9

[Andreas Franke (gone)]

No, this is not fixed yet. Other builds tested (all crashing): 3/29, 5/27.

Comment 10

[Mike]

M16 has been out for a while now, these bugs target milestones need to be 
updated.

Comment 11

[Jan Carpenter]

Using Andreas Franke's xml testcase, this is crashing instantly on all platforms
2000-08-22-08-M18 : Win32
2000-08-22-12-M18 : Linux
2000-08-22-10-M18 : Mac

Comment 12

[rickg]

This appears to *ALWAYS* crash in expat. Major priority. Marking nsbeta3+, and 
reassigning to nisheeth.

Comment 13

[clayton]

pdt - helping Nisheeth set his priorities.

Comment 14

[Nisheeth Ranjan]

I am not going to be able to get to this for beta 3.  Marking nsbeta3-,
helpwanted, relnote.

Comment 15

[Jan Carpenter]

Still crashing 
2000-10-05-09-MN6 : Windows
2000-10-05-13-MN6 : Mac

Nominating for RTM

Comment 16

[Arne de Bruijn]

Attachment: patch to avoid abort() in expat on invalid % in entity value

Comment 17

[Arne de Bruijn]

Attachment: patch with fixes from expat 1.2

Comment 18

[Arne de Bruijn]

I attached a patch to prevent the abort() in expat with entity values containing 
% followed by % or whitespace, and return an error instead.

I also attached a patch with the fixes in expat 1.2, maybe those should be 
checked in too.

Comment 19

[Nisheeth Ranjan]

This is a relatively safe and localized fix for a crasher.  I recognize that
this is a crasher that only happens on erroneous XML content, but now that we
have an external patch that fixes this crash, I'd really appreciate if the PDT
rtm++'d this bug.

To start that process, ccing Vidur to get a super review and marking the bug
[rtm need info] for now.

Comment 20

[Nisheeth Ranjan]

r=nisheeth.

Comment 21

[vidur (gone)]

a=vidur. We should probably feed the first patch back to the expat owners.

Comment 22

[Nisheeth Ranjan]

Marking rtm+ to get onto the PDT radar.

Comment 23

[selmer (gone)]

rtm-, not likely to hit this, it's an incorrect document.  Is there a one-line
fix that removes just the crash?

Comment 24

[Nisheeth Ranjan]

The patch timestamped "10/08/00 08:28" is a four line fix for just the crash.
We are asking permission to check in that patch, not the latest one timestamped
"10/08/00 08:30".

Renominating rtm+.

Comment 25

[Phil Peterson]

Is this going to hit any real users out in the real world? And, are there other
tokens which would cause us to drop into the default/abort case (I can't believe
we really have an abort() call in here)? If this is really going to help real
users, we'll take it, but if this is a mozilla-developer-only thing, then we
don't need it on the RTM branch. Marking [rtm need info]

Comment 26

[Nisheeth Ranjan]

Yes, this is a mozilla developer only thing.  I'd argue that Netscape 6 is a 
platform for mozilla and web developers and since this fix is low risk it should 
be allowed.  At the same time, I can understand the reasons why additional 
control of the tree is needed so close to the cutting of the rtm candidate 
builds.

I've checked the first patch into the trunk.  I'll run both patches by the expat 
owners before checking in further changes to expat.

-> rtm-

Comment 27

[Frank Tang]

I think we need to fix the crash. I am not sure the patch is the right thing to
do. But if user hit something like
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=9281 , we should not crash.

Comment 28

[Frank Tang]

a very very safe patch which will let the mozilla won't crash when we hit bad
xml from the net
===================================================================
RCS file: /m/pub/mozilla/expat/xmlparse/xmlparse.c,v
retrieving revision 1.12
diff -u -2 -r1.12 xmlparse.c
--- xmlparse.c  2000/08/30 01:35:07     1.12
+++ xmlparse.c  2000/10/18 01:14:46
@@ -447,4 +447,5 @@
 #endif

+#define abort() printf("someone call abort() from expat\n")
 XML_Parser XML_ParserCreate(const XML_Char *encodingName)
 {

basically, I make a macro to let it simply printf some message and then continue.

Comment 29

[Nisheeth Ranjan]

I would argue that this is not the right way to fix this bug.  It is better to
crash on certain cases of malformed XML content than to parse that content
without giving user feedback, which is essentially what would happen with this
patch, because the printf() would end up writing its error message into a hidden
console window that is never seen by the user.

The right fix, in my opinion, is to add support for a new error and return it
instead of calling abort() so that the error shows up in the browser window.  I
don't think this work will have sufficient user impact to make it into RTM.
But, we should definitely attack this post RTM.

I'm leaving this bug as rtm-.  Frank, in case you want us to reconsider this
decision, please remove the rtm- from the status whiteboard so that it appears
on our rtm nominated bug list.  Thanks!

Comment 30

[Frank Tang]

>It is better to crash on certain cases of malformed XML content than to parse
that content without giving user feedback
I don't think there are anything worst than CRASH.
I do agree with you that you may find a better fix post rtm. But I don't think
we can ship without fixing the crash. If I spam all CPD with the
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=9281 then everyone read
that message will crash the mail. I think that is not acceptable.
With my patch, the user will still see some parser error on the screen with the
2nd attachment.

I do agree that you should spend more time to produce a real fix post rtm.
But... we have no time now. .
Remove rtm-

Comment 31

[Frank Tang]

I have a better fix here

Index: xmltok/xmldef.h
===================================================================
RCS file: /m/pub/mozilla/expat/xmltok/xmldef.h,v
retrieving revision 1.15
diff -u -3 -0 -r1.15 xmldef.h
--- xmldef.h    2000/08/30 00:17:07     1.15
+++ xmldef.h    2000/10/19 19:26:53
@@ -8,45 +8,46 @@
 #ifdef XML_WINLIB

 #define WIN32_LEAN_AND_MEAN
 #define STRICT
 #include <windows.h>

 #define malloc(x) HeapAlloc(GetProcessHeap(), 0, (x))
 #define calloc(x, y) HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, (x)*(y))
 #define free(x) HeapFree(GetProcessHeap(), 0, (x))
 #define realloc(x, y) HeapReAlloc(GetProcessHeap(), 0, x, y)
 #define abort() /* as nothing */

 #else /* not XML_WINLIB */

 #include <stdlib.h>

 #endif /* not XML_WINLIB */

 /* This file can be used for any definitions needed in
 particular environments. */

 /* Mozilla specific defines */

 #ifdef MOZILLA_CLIENT

 #include "nspr.h"
 #define malloc(x) PR_Malloc((size_t)(x))
 #define realloc(x, y) PR_Realloc((x), (size_t)(y))
 #define calloc(x, y) PR_Calloc((x),(y))
 #define free(x) PR_Free(x)
+#define abort() /* as nothing */
 #if PR_BYTES_PER_INT != 4
 #define int int32

samething. define nothing for abort. however, if you look at the define in the
ifdef XML_WINLIB section, you will notice it already define abort() to nothing
in there.

Comment 32

[Frank Tang]

>Is this going to hit any real users out in the real world?
Phil- It will hit you if anyone forward
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=9281 to you in a email
message to attack you. 

Comment 33

[Frank Tang]

>parse that content without giving user feedback
This is a false assumption. If you try any of the #define abort() to nothing fix
in your local tree, you will notice that the xml parser WILL tell the user
something is wrong LATER. The xmlparser may not give the feedback in that
particular spot. But since the syntax is bad it is likely to give feedback to
users for following problem.

Comment 34

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I just found that the Expat project on Sourceforge has a new version of Expat
available, and the comments say they fixed some obscuce % handling issue. Maybe
this was fixed...

Comment 35

[Nisheeth Ranjan]

I think that Frank's one line fix to xmldef.h pasted in his comment timestamped
"2000-10-19 13:05" is very low risk.  It fixes the crash in the XML parser when
it encounters certain types of malformed XML content.  The fix also does display
an XML error message in the browser window.  I had incorrectly assumed from his
earlier patch that the fix would not give any feedback about the error to the user.

This bug will be encountered by end users if malicious web sites post certain
types of malformed XML pages or malicious spammers email such a malformed XML
page to a user.  This bug will be encountered by web developers as they develop
XML pages if they create such a malformed XML page and load it in Netscape 6.

I leave it to the PDT to decide whether this is a ship stopper.  I think that
the patch is extremely low risk and should only make things better (in cases
where we crash now, we won't crash at all or maybe end up crashing somewhere
later in the code path).  At the same time, I recognize that there are many ways
in which malicious users can create pages that crash the Netscape 6 browser and
that we can't delay the 6.0 release to fix all such crashers, especially, a
crasher caused by malformed XML content.  Since this fix is in hand, however,
and because it can only improve things and prevent crashes, I urge the PDT to
let this in.

Frank, please add any more comments you see fit.  Thanks!

Comment 36

[Nisheeth Ranjan]

Adding frank to the cc list.  Frank, please see my earlier comment and add
anything you deem appropriate.  Thanks!

I'll catch Vidur and ask him to super review this today and mark this rtm+ after
that.

Comment 37

[vidur (gone)]

Heh! Without Frank's patch, expat sometimes calls the libc abort() function -
definitely a bad thing. I'd strongly recommend we take Frank's change for rtm.
It prevents us from taking a codepath that will always bring down the
application. Admittedly, there aren't many pages that currently exist on the web
that would cause this to happen, but Frank's patch represents a safe way to
future proof against such malformed pages.

sr=vidur

Comment 38

[Nisheeth Ranjan]

Now that a review and super review of the patch is complete, marking rtm+.

Comment 39

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I would also like to point out that the latest Expat release has changed all
abort()s to error return values. I am not sure if that is going to be of any
help to us, since the latest Expat code is under some MIT open source license.
Maybe this doesn't matter at all if we are going to replace Expat with something
else eventually. Anyway, we can't integrate the latest Expat into RTM so this is
just to inform people of the possibilities...

Comment 40

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Doh. Bugzilla cleared the [rtm+], I am putting it back.

Comment 41

[Gervase Markham [:gerv]]

Marking relnote-devel in case this doesn't make it in.

Gerv

Comment 42

[selmer (gone)]

#define abort() to a null statement has potentially wide impact.  We can't 
accept this fix until we know what that impact is.  Someone needs to identify 
every abort statement which will be affected by this change and ensure that this 
is the proper behavior for each instance.  We are the most interested in the 
single abort that was discovered for this bug.

A minimal fix would be to ONLY change the abort identified for this bug so this 
becomes a one-instance, one-line fix.

Comment 43

[Frank Tang]

[e-mail]- can you please review and confirm that all the 4 abort() in the
xmlparse.c is ok to not calling abort() ?

Comment 44

[Frank Tang]

Here are the 4 abort() in xmlparse.c
line 1973 doCdataSection
1889 static
1890 enum XML_Error doCdataSection(XML_Parser parser,
1891                               const ENCODING *enc,
1892                               const char **startPtr,
1893                               const char *end,
1894                               const char **nextPtr)
1895 {
1896   const char *s = *startPtr;
1897   const char **eventPP;
1898   const char **eventEndPP;
1899   if (enc == encoding) {
1900     eventPP = &eventPtr;
1901     *eventPP = s;
1902     eventEndPP = &eventEndPtr;
1903   }
1904   else {
1905     eventPP = &(openInternalEntities->internalEventPtr);
1906     eventEndPP = &(openInternalEntities->internalEventEndPtr);
1907   }
1908   *eventPP = s;
1909   *startPtr = 0;
1910   for (;;) {
1911     const char *next;
1912     int tok = XmlCdataSectionTok(enc, s, end, &next);
1913     *eventEndPP = next;
1914     switch (tok) {
1915     case XML_TOK_CDATA_SECT_CLOSE:
1916       if (endCdataSectionHandler)
1917         endCdataSectionHandler(handlerArg);
1918 #if 0
1919       /* see comment under XML_TOK_CDATA_SECT_OPEN */
1920       else if (characterDataHandler)
1921         characterDataHandler(handlerArg, dataBuf, 0);
1922 #endif
1923       else if (defaultHandler)
1924         reportDefault(parser, enc, s, next);
1925       *startPtr = next;
1926       return XML_ERROR_NONE;
1927     case XML_TOK_DATA_NEWLINE:
1928       if (characterDataHandler) {
1929         XML_Char c = 0xA;
1930         characterDataHandler(handlerArg, &c, 1);
1931       }
1932       else if (defaultHandler)
1933         reportDefault(parser, enc, s, next);
1934       break;
1935     case XML_TOK_DATA_CHARS:
1936       if (characterDataHandler) {
1937         if (MUST_CONVERT(enc, s)) {
1938           for (;;) {
1939             ICHAR *dataPtr = (ICHAR *)dataBuf;
1940             XmlConvert(enc, &s, next, &dataPtr, (ICHAR *)dataBufEnd);
1941             *eventEndPP = next;
1942             characterDataHandler(handlerArg, dataBuf, dataPtr - (ICHAR 
*)dataBuf);
1943             if (s == next)
1944               break;
1945             *eventPP = s;
1946           }
1947         }
1948         else
1949           characterDataHandler(handlerArg,
1950                                (XML_Char *)s,
1951                                (XML_Char *)next - (XML_Char *)s);
1952       }
1953       else if (defaultHandler)
1954         reportDefault(parser, enc, s, next);
1955       break;
1956     case XML_TOK_INVALID:
1957       *eventPP = next;
1958       return XML_ERROR_INVALID_TOKEN;
1959     case XML_TOK_PARTIAL_CHAR:
1960       if (nextPtr) {
1961         *nextPtr = s;
1962         return XML_ERROR_NONE;
1963       }
1964       return XML_ERROR_PARTIAL_CHAR;
1965     case XML_TOK_PARTIAL:
1966     case XML_TOK_NONE:
1967       if (nextPtr) {
1968         *nextPtr = s;
1969         return XML_ERROR_NONE;
1970       }
1971       return XML_ERROR_UNCLOSED_CDATA_SECTION;
1972     default:
1973       abort();
1974     }
1975     *eventPP = s = next;
1976   }
1977   /* not reached */
1978 }
1979 

line 2050 in doIgnoreSection
2002 static
2003 enum XML_Error doIgnoreSection(XML_Parser parser,
2004                                const ENCODING *enc,
2005                                const char **startPtr,
2006                                const char *end,
2007                                const char **nextPtr)
2008 {
2009   const char *next;
2010   int tok;
2011   const char *s = *startPtr;
2012   const char **eventPP;
2013   const char **eventEndPP;
2014   if (enc == encoding) {
2015     eventPP = &eventPtr;
2016     *eventPP = s;
2017     eventEndPP = &eventEndPtr;
2018   }
2019   else {
2020     eventPP = &(openInternalEntities->internalEventPtr);
2021     eventEndPP = &(openInternalEntities->internalEventEndPtr);
2022   }
2023   *eventPP = s;
2024   *startPtr = 0;
2025   tok = XmlIgnoreSectionTok(enc, s, end, &next);
2026   *eventEndPP = next;
2027   switch (tok) {
2028   case XML_TOK_IGNORE_SECT:
2029     if (defaultHandler)
2030       reportDefault(parser, enc, s, next);
2031     *startPtr = next;
2032     return XML_ERROR_NONE;
2033   case XML_TOK_INVALID:
2034     *eventPP = next;
2035     return XML_ERROR_INVALID_TOKEN;
2036   case XML_TOK_PARTIAL_CHAR:
2037     if (nextPtr) {
2038       *nextPtr = s;
2039       return XML_ERROR_NONE;
2040     }
2041     return XML_ERROR_PARTIAL_CHAR;
2042   case XML_TOK_PARTIAL:
2043   case XML_TOK_NONE:
2044     if (nextPtr) {
2045       *nextPtr = s;
2046       return XML_ERROR_NONE;
2047     }
2048     return XML_ERROR_SYNTAX; /* XML_ERROR_UNCLOSED_IGNORE_SECTION */
2049   default:
2050     abort();
2051   }
2052   /* not reached */
2053 }
2054 

line 2941 in appendAttributeValue
2828 static enum XML_Error
2829 appendAttributeValue(XML_Parser parser, const ENCODING *enc, int isCdata,
2830                      const char *ptr, const char *end,
2831                      STRING_POOL *pool)
2832 {
2833   for (;;) {
2834     const char *next;
2835     int tok = XmlAttributeValueTok(enc, ptr, end, &next);
2836     switch (tok) {
2837     case XML_TOK_NONE:
2838       return XML_ERROR_NONE;
2839     case XML_TOK_INVALID:
2840       if (enc == encoding)
2841         eventPtr = next;
2842       return XML_ERROR_INVALID_TOKEN;
2843     case XML_TOK_PARTIAL:
2844       if (enc == encoding)
2845         eventPtr = ptr;
2846       return XML_ERROR_INVALID_TOKEN;
2847     case XML_TOK_CHAR_REF:
2848       {
2849         XML_Char buf[XML_ENCODE_MAX];
2850         int i;
2851         int n = XmlCharRefNumber(enc, ptr);
2852         if (n < 0) {
2853           if (enc == encoding)
2854             eventPtr = ptr;
2855           return XML_ERROR_BAD_CHAR_REF;
2856         }
2857         if (!isCdata
2858             && n == 0x20 /* space */
2859             && (poolLength(pool) == 0 || poolLastChar(pool) == 0x20))
2860           break;
2861         n = XmlEncode(n, (ICHAR *)buf);
2862         if (!n) {
2863           if (enc == encoding)
2864             eventPtr = ptr;
2865           return XML_ERROR_BAD_CHAR_REF;
2866         }
2867         for (i = 0; i < n; i++) {
2868           if (!poolAppendChar(pool, buf[i]))
2869             return XML_ERROR_NO_MEMORY;
2870         }
2871       }
2872       break;
2873     case XML_TOK_DATA_CHARS:
2874       if (!poolAppend(pool, enc, ptr, next))
2875         return XML_ERROR_NO_MEMORY;
2876       break;
2877       break;
2878     case XML_TOK_TRAILING_CR:
2879       next = ptr + enc->minBytesPerChar;
2880       /* fall through */
2881     case XML_TOK_ATTRIBUTE_VALUE_S:
2882     case XML_TOK_DATA_NEWLINE:
2883       if (!isCdata && (poolLength(pool) == 0 || poolLastChar(pool) == 
0x20))
2884         break;
2885       if (!poolAppendChar(pool, 0x20))
2886         return XML_ERROR_NO_MEMORY;
2887       break;
2888     case XML_TOK_ENTITY_REF:
2889       {
2890         const XML_Char *name;
2891         ENTITY *entity;
2892         XML_Char ch = XmlPredefinedEntityName(enc,
2893                                               ptr + enc->minBytesPerChar,
2894                                               next - enc->minBytesPerChar);
2895         if (ch) {
2896           if (!poolAppendChar(pool, ch))
2897             return XML_ERROR_NO_MEMORY;
2898           break;
2899         }
2900         name = poolStoreString(&temp2Pool, enc,
2901                                ptr + enc->minBytesPerChar,
2902                                next - enc->minBytesPerChar);
2903         if (!name)
2904           return XML_ERROR_NO_MEMORY;
2905         entity = (ENTITY *)lookup(&dtd.generalEntities, name, 0);
2906         poolDiscard(&temp2Pool);
2907         if (!entity) {
2908           if (dtd.complete) {
2909             if (enc == encoding)
2910               eventPtr = ptr;
2911             return XML_ERROR_UNDEFINED_ENTITY;
2912           }
2913         }
2914         else if (entity->open) {
2915           if (enc == encoding)
2916             eventPtr = ptr;
2917           return XML_ERROR_RECURSIVE_ENTITY_REF;
2918         }
2919         else if (entity->notation) {
2920           if (enc == encoding)
2921             eventPtr = ptr;
2922           return XML_ERROR_BINARY_ENTITY_REF;
2923         }
2924         else if (!entity->textPtr) {
2925           if (enc == encoding)
2926             eventPtr = ptr;
2927           return XML_ERROR_ATTRIBUTE_EXTERNAL_ENTITY_REF;
2928         }
2929         else {
2930           enum XML_Error result;
2931           const XML_Char *textEnd = entity->textPtr + entity->textLen;
2932           entity->open = 1;
2933           result = appendAttributeValue(parser, internalEncoding, isCdata, 
(char *)entity->textPtr, (char *)textEnd, pool);
2934           entity->open = 0;
2935           if (result)
2936             return result;
2937         }
2938       }
2939       break;
2940     default:
2941       abort();
2942     }
2943     ptr = next;
2944   }
2945   /* not reached */
2946 }

and line 3050 in storeEntityValue
2949 enum XML_Error storeEntityValue(XML_Parser parser,
2950                                 const ENCODING *enc,
2951                                 const char *entityTextPtr,
2952                                 const char *entityTextEnd)
2953 {
2954   STRING_POOL *pool = &(dtd.pool);
2955   for (;;) {
2956     const char *next;
2957     int tok = XmlEntityValueTok(enc, entityTextPtr, entityTextEnd, &next);
2958     switch (tok) {
2959     case XML_TOK_PARAM_ENTITY_REF:
2960 #ifdef XML_DTD
2961       if (parentParser || enc != encoding) {
2962         enum XML_Error result;
2963         const XML_Char *name;
2964         ENTITY *entity;
2965         name = poolStoreString(&tempPool, enc,
2966                                entityTextPtr + enc->minBytesPerChar,
2967                                next - enc->minBytesPerChar);
2968         if (!name)
2969           return XML_ERROR_NO_MEMORY;
2970         entity = (ENTITY *)lookup(&dtd.paramEntities, name, 0);
2971         poolDiscard(&tempPool);
2972         if (!entity) {
2973           if (enc == encoding)
2974             eventPtr = entityTextPtr;
2975           return XML_ERROR_UNDEFINED_ENTITY;
2976         }
2977         if (entity->open) {
2978           if (enc == encoding)
2979             eventPtr = entityTextPtr;
2980           return XML_ERROR_RECURSIVE_ENTITY_REF;
2981         }
2982         if (entity->systemId) {
2983           if (enc == encoding)
2984             eventPtr = entityTextPtr;
2985           return XML_ERROR_PARAM_ENTITY_REF;
2986         }
2987         entity->open = 1;
2988         result = storeEntityValue(parser,
2989                                   internalEncoding,
2990                                   (char *)entity->textPtr,
2991                                   (char *)(entity->textPtr + 
entity->textLen));
2992         entity->open = 0;
2993         if (result)
2994           return result;
2995         break;
2996       }
2997 #endif /* XML_DTD */
2998       eventPtr = entityTextPtr;
2999       return XML_ERROR_SYNTAX;
3000     case XML_TOK_NONE:
3001       return XML_ERROR_NONE;
3002     case XML_TOK_ENTITY_REF:
3003     case XML_TOK_DATA_CHARS:
3004       if (!poolAppend(pool, enc, entityTextPtr, next))
3005         return XML_ERROR_NO_MEMORY;
3006       break;
3007     case XML_TOK_TRAILING_CR:
3008       next = entityTextPtr + enc->minBytesPerChar;
3009       /* fall through */
3010     case XML_TOK_DATA_NEWLINE:
3011       if (pool->end == pool->ptr && !poolGrow(pool))
3012         return XML_ERROR_NO_MEMORY;
3013       *(pool->ptr)++ = 0xA;
3014       break;
3015     case XML_TOK_CHAR_REF:
3016       {
3017         XML_Char buf[XML_ENCODE_MAX];
3018         int i;
3019         int n = XmlCharRefNumber(enc, entityTextPtr);
3020         if (n < 0) {
3021           if (enc == encoding)
3022             eventPtr = entityTextPtr;
3023           return XML_ERROR_BAD_CHAR_REF;
3024         }
3025         n = XmlEncode(n, (ICHAR *)buf);
3026         if (!n) {
3027           if (enc == encoding)
3028             eventPtr = entityTextPtr;
3029           return XML_ERROR_BAD_CHAR_REF;
3030         }
3031         for (i = 0; i < n; i++) {
3032           if (pool->end == pool->ptr && !poolGrow(pool))
3033             return XML_ERROR_NO_MEMORY;
3034           *(pool->ptr)++ = buf[i];
3035         }
3036       }
3037       break;
3038     case XML_TOK_PARTIAL:
3039       if (enc == encoding)
3040         eventPtr = entityTextPtr;
3041       return XML_ERROR_INVALID_TOKEN;
3042     case XML_TOK_INVALID:
3043       if (enc == encoding)
3044         eventPtr = next;
3045       return XML_ERROR_INVALID_TOKEN;
3046     case XML_TOK_PERCENT:
3047       if (enc == encoding)
3048        eventPtr = next;
3049       return XML_ERROR_SYNTAX;
3050     default:
3051       abort();
3052     }
3053     entityTextPtr = next;
3054   }
3055   /* not reached */
3056 }

Comment 45

[Frank Tang]

So... these 4 abort() are called when the parser go unexpect token in 
doCdataSection , storeEntityValue , doIgnoreSection and appendAttributeValue

Comment 46

[Frank Tang]

http://bugzilla.mozilla.org/showattachment.cgi?attach_id=9281 (the 2nd 
attachment) hit the case of storeEntityValue

Comment 47

[Frank Tang]

cannot find a case which will reach the abort() code in doCdataSection() . Try 
http://ftang/ftang/tmp/cdata.xml - this file won't crash it, but basically that 
part of text is handle by doCdataSection()

Comment 48

[Frank Tang]

Since I am not xml expert, I cannot create test for the doIgnroeSection abort() 
neither.

Comment 49

[Frank Tang]

It seems all the 4 cases are hitting illegal token type when the xmlparser parse 
in different xml section. I suggest we take the #define patch as is unless 
someone can tell me these 4 cases are deal with symbol tables and have security 
concern. The other way to fix is is to replace these 4 places with

return XML_ERROR_NONE;

Comment 50

[Frank Tang]

sorry, I take it back, Probably should use other error code instead of 
XML_ERROR_NONE
probably 
return XML_ERROR_SYNTAX;

Comment 51

[Frank Tang]

Another point of we should take the #define abort() {} patch is currently Linux 
version won't crash because we already patch out abort() to do nothing somehow. 
Although the xmlparse.c will call abort() . The implementatoin of abort() are 
patched out by some code (I cannot find where is it, but it won't exit)

Comment 52

[James Clark]

xmlparse.c calls abort() for internal logic errors.  If you remove the calls to 
abort(), bugs may go undetected.  It would be better to replace them by whatever 
Mozilla uses elsewhere for assertion failures (preferably by a definition in 
xmldef.h).

For the specific case of % in the DTD, that's a bug in entityValueTok in 
xmltok_impl.c.  It shouldn't be returning TOK_PERCENT at all.  This has been 
fixed in the current expat sources (on sourceforge.net).

Comment 53

[James Clark]

Attachment: Fix to entityValueTok for problem with %

Comment 54

[vidur (gone)]

sr=vidur for James Clark's patch.

Defining abort() to be a noop isn't the right long-term fix, I agree. We still
might want to consider it for rtm, though. Bugs may go undetected, but that's
definitely better than unconditionally crashing. Post-rtm, Heikki or Nisheeth
should consider a better solution for dealing with these error cases.

Comment 55

[Nisheeth Ranjan]

r=nisheeth for James Clark's patch.  Marking rtm+ for PDT reconsideration...

Comment 56

[Nisheeth Ranjan]

Please let me know whether you will allow

a) James Clark's patch
b) the redefinition of abort() to a noop

into the rtm branch.

Thanks!

Comment 57

[selmer (gone)]

rtm-, not ship stopper.  Thanks for getting this, let's land it on the trunk 
right away.

Comment 58

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Stealing...

Comment 59

[Heikki Toivonen (remove -bugzilla when emailing directly)]

James' patch checked in. Marking fixed.

Comment 60

[Jan Carpenter]

verified:
2001-01-22-08-Mtrunk : win32
2001-01-19-11-Mtrunk : linux
2001-01-22-04-Mtrunk : mac

Comment 61

[Jesse Ruderman]

Crashtest added as part of http://hg.mozilla.org/mozilla-central/rev/5a6def05ccbc