607721 - XML_Parse returns undefined value in test_bug592829.html

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[Julian Seward [:jseward]]

M-C of 19 Oct, MSVC9, x86, on content/base/test/test_bug592829.html

 Conditional jump or move depends on uninitialised value(s)
    at 0x10046A26: nsExpatDriver::ParseBuffer (nsexpatdriver.cpp:1022)
    by 0x10046BD0: nsExpatDriver::ConsumeToken (nsexpatdriver.cpp:1101)
    by 0x100579F6: nsParser::Tokenize (nsparser.cpp:3090)
    by 0x10056010: nsParser::ResumeParse (nsparser.cpp:2322)
    by 0x10056CDE: nsParser::OnDataAvailable (nsparser.cpp:2954)
    by 0x105C747D: nsDOMParser::ParseFromStream (nsdomparser.cpp:308)
    by 0x105C6B0D: nsDOMParser::ParseFromString (nsdomparser.cpp:158)
    by 0x10CCC276: NS_InvokeByIndex_P (xptcinvoke.cpp:102)
    by 0x10A7E557: CallMethodHelper::Invoke (xpcwrappednative.cpp:3054)
    by 0x10A7C979: CallMethodHelper::Call (xpcwrappednative.cpp:2321)
    by 0x10A7C766: XPCWrappedNative::CallMethod (xpcwrappednative.cpp:2285)
    by 0x10A6F862: XPC_WN_CallMethod (xpcwrappednativejsops.cpp:1631)
    by 0x7F328ADA: js::CallJSNative (???:652)
    by 0x7F42C349: CallCompiler::generateNativeStub (???:435)
    by 0x7F42C225: js::mjit::ic::NativeCall (???:659)
    by 0x471E4A78: ???
  Uninitialised value was created by a stack allocation
    at 0x103AD703: MOZ_XML_Parse (xmlparse.c:1476)


Analysis:

parser/htmlparser/src/nsExpatDriver.cpp has this

  1001        status = XML_Parse(mExpatParser,
  1002                           reinterpret_cast<const char*>(aBuffer),
  1003                           aLength * sizeof(PRUnichar), aIsFinal);
  ...
  1022      if (status == XML_STATUS_ERROR) {
  1023        mInternalState = NS_ERROR_HTMLPARSER_STOPPARSING;
  1024      }

So 'status' is undefined on line 1022.  Looking at the source for
XML_Parse, in parser/expat/lib/xmlparse.c we have

  1519  #ifndef XML_CONTEXT_BYTES
  1520    else if (bufferPtr == bufferEnd) {
  1521      const char *end;
  1522      int nLeftOver;
  1523      enum XML_Error result;

It is possible to get from line 1523 to a 'return result;' with no
assignment to 'result'.  This is easily demonstrated by changing 1523
to read

  1523      enum XML_Error result = (enum XML_Error)9876;

and in nsExpatDriver.cpp, printing the returned value

  1022      printf("XXXXXXX1 status = %u\n", (int)status);
  1023      if (status == XML_STATUS_ERROR) {
  1024        mInternalState = NS_ERROR_HTMLPARSER_STOPPARSING;
  1025      }

When running /test_bug592829.html, this prints a whole bunch of

  XXXXXXX1 status = 1

but then a

  XXXXXXX1 status = 9876

Comment 1

[Julian Seward [:jseward]]

Possible bogus path in xmlparse.c:

  1523      enum XML_Error result;
  1524      parseEndByteIndex += len;
  1525      positionPtr = s;
  1526      ps_finalBuffer = (XML_Bool)isFinal;
  1527  
  1528      errorCode = processor(parser, s, parseEndPtr = s + len, &end);
  1529  

suppose 
 errorCode == XML_ERROR_NONE
 and
 ps_parsing is neither XML_SUSPENDED, XML_INITIALIZED nor XML_PARSING

  1530      if (errorCode != XML_ERROR_NONE) {
  ....        //// not taken
  1534      }
  1535      else {
  1536        switch (ps_parsing) {
              //// no case taken, fall off end of case
  1537        case XML_SUSPENDED:
  1538          result = XML_STATUS_SUSPENDED;
  1539          break;
  1540        case XML_INITIALIZED:
  1541        case XML_PARSING:
  1542          result = XML_STATUS_OK;
  1543          if (isFinal) {
  1544            ps_parsing = XML_FINISHED;
  1545            return result;
  1546          }
  1547        }
  1548      }

result is still uninitialised

  1550      XmlUpdatePosition(encoding, positionPtr, end, &position);
  1551      nLeftOver = s + len - end;

nLeftOver == 0

  1552      if (nLeftOver) {
  ....        //// not taken
  1573      }
  1574      bufferPtr = buffer;
  1575      bufferEnd = buffer + nLeftOver;
  1576      positionPtr = bufferPtr;
  1577      parseEndPtr = bufferEnd;
  1578      eventPtr = bufferPtr;
  1579      eventEndPtr = bufferPtr;
  1580      return result;  // gark

In fact it is possible for result to be uninitialised even if
nLeftOver != 0 provided that !(buffer == NULL || nLeftOver > bufferLim - buffer)

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 3

[Julian Seward [:jseward]]

(In reply to comment #2)
> Created attachment 489174 [details] [diff] [review]

Yes, that appears to stop Valgrind complaining.

Comment 4

[Peter Van der Beken [:peterv]]

Filed as https://sourceforge.net/tracker/?func=detail&aid=3206497&group_id=10127&atid=110127

Comment 5

[Peter Van der Beken [:peterv]]

Comment on attachment 489174 [details] [diff] [review]
v1

Let's go with this for now, we can always change if upstream wants to fix it differently.

Comment 6

[Peter Van der Beken [:peterv]]

http://hg.mozilla.org/mozilla-central/rev/3002f08d9420