[Expat-bugs] [ expat-Bugs-1584898 ] (freed) memory access

[SourceForge.net]

Bugs item #1584898, was opened at 2006-10-26 03:08
Message generated for change (Comment added) made by kwaclaw
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=110127&aid=1584898&group_id=10127

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: None
Group: None
>Status: Closed
>Resolution: Rejected
Priority: 5
Private: No
Submitted By: Nobody/Anonymous (nobody)
Assigned to: Nobody/Anonymous (nobody)
Summary: (freed) memory access

Initial Comment:
There is a bug in latest and previous versions of expat. Following 
procedure may be accessing already freed memory under some 
cicrumstances (depending on malloc implementation and/or block size 
being re-allocated)

I marked the offending lines with "### BUG -->" prefix

/* Initially tag->rawName always points into the parse buffer;
   for those TAG instances opened while the current parse buffer was
   processed, and not yet closed, we need to store tag->rawName in a 
more
   permanent location, since the parse buffer is about to be discarded.
*/
static XML_Bool
storeRawNames(XML_Parser parser)
{
  TAG *tag = tagStack;
  while (tag) {
    int bufSize;
    int nameLen = sizeof(XML_Char) * (tag->name.strLen + 1);
    char *rawNameBuf = tag->buf + nameLen;
    /* Stop if already stored.  Since tagStack is a stack, we can stop
       at the first entry that has already been copied; everything
       below it in the stack is already been accounted for in a
       previous call to this function.
    */
    if (tag->rawName == rawNameBuf)
      break;
    /* For re-use purposes we need to ensure that the
       size of tag->buf is a multiple of sizeof(XML_Char).
    */
    bufSize = nameLen + ROUND_UP(tag->rawNameLength, sizeof
(XML_Char));
    if (bufSize > tag->bufEnd - tag->buf) {
      char *temp = (char *)REALLOC(tag->buf, bufSize);
      if (temp == NULL)
        return XML_FALSE;
      /* if tag->name.str points to tag->buf (only when namespace
         processing is off) then we have to update it
      */

      ### ABOVE REALLOC may return different memory block so 
following tag->buf access may become invalid

      "### BUG -->" if (tag->name.str == (XML_Char *)tag->buf)
        tag->name.str = (XML_Char *)temp;


      /* if tag->name.localPart is set (when namespace processing is on)
         then update it as well, since it will always point into tag->buf
      */
      if (tag->name.localPart)
        tag->name.localPart = (XML_Char *)temp + (tag->name.localPart 
-
                                                  (XML_Char *)tag->buf);
Following statement should have been performed just after REALLOC
"### BUG -->"       tag->buf = temp;
      tag->bufEnd = temp + bufSize;
      rawNameBuf = temp + nameLen;
    }
    memcpy(rawNameBuf, tag->rawName, tag->rawNameLength);
    tag->rawName = rawNameBuf;
    tag = tag->parent;
  }
  return XML_TRUE;
}


----------------------------------------------------------------------

>Comment By: Karl Waclawek (kwaclaw)
Date: 2006-11-25 12:36

Message:
Logged In: YES 
user_id=290026
Originator: NO

Rejecting this bug report - see my last message.

----------------------------------------------------------------------

Comment By: Karl Waclawek (kwaclaw)
Date: 2006-10-27 11:43

Message:
Logged In: YES 
user_id=290026

Your code introduces a bug in the line after if
"(tag->name.localPart)",
as it uses the new and not the old value of tag->buf.

Apart from that, I do not see why we should rewrite the
Expat code
to compensate for the deficiencies of some debugging tools.

----------------------------------------------------------------------

Comment By: Nobody/Anonymous (nobody)
Date: 2006-10-26 22:03

Message:
Logged In: NO 

hi,

you are right the program will never crash, but the verification tools
will complain about it as the pointer being accessed is no longer valid.
Anyhow it would be better if the same procedure is written in the
followiing way, 
so that it is not "flaged" as problematic by subsequent tools (e.g. Bounds
Checker, Purify, etc...)

/* Initially tag->rawName always points into the parse buffer;
   for those TAG instances opened while the current parse buffer was
   processed, and not yet closed, we need to store tag->rawName in a more
   permanent location, since the parse buffer is about to be discarded.
*/
static XML_Bool
storeRawNames(XML_Parser parser)
{
  TAG *tag = tagStack;
  while (tag) {
    int bufSize;
    int nameLen = sizeof(XML_Char) * (tag->name.strLen + 1);
    char *rawNameBuf = tag->buf + nameLen;
    /* Stop if already stored.  Since tagStack is a stack, we can stop
       at the first entry that has already been copied; everything
       below it in the stack is already been accounted for in a
       previous call to this function.
    */
    if (tag->rawName == rawNameBuf)
      break;
    /* For re-use purposes we need to ensure that the
       size of tag->buf is a multiple of sizeof(XML_Char).
    */
    bufSize = nameLen + ROUND_UP(tag->rawNameLength, sizeof(XML_Char));
    if (bufSize > tag->bufEnd - tag->buf) {
 
	/* mj [27-10-2006]
	  */ XML_Bool updateTagName = tag->name.str == (XML_Char *)tag->buf;
		
	  char *temp = (char *)REALLOC(tag->buf, bufSize);
      if (temp == NULL)
        return XML_FALSE;
		
	/* mj [26-10-2006] - due to REALLOC statement tag->buf may become
invalid, therefore i
	 * update the pointer before subsequent accesses occur to it
	 */ tag->buf = temp;
		
      /* if tag->name.str points to tag->buf (only when namespace
         processing is off) then we have to update it
      */
      if (updateTagName /* mj [27-10-2006] tag->name.str == (XML_Char
*)tag->buf */ )
        tag->name.str = (XML_Char *)temp;
      /* if tag->name.localPart is set (when namespace processing is on)
         then update it as well, since it will always point into tag->buf
      */
      if (tag->name.localPart)
        tag->name.localPart = (XML_Char *)temp + (tag->name.localPart -
                                                  (XML_Char *)tag->buf);
      /* mj [26-10-2006] - removed (see above) following statement */
	  /* tag->buf = temp; */
	  
      tag->bufEnd = temp + bufSize;
      rawNameBuf = temp + nameLen;
    }
    memcpy(rawNameBuf, tag->rawName, tag->rawNameLength);
    tag->rawName = rawNameBuf;
    tag = tag->parent;
  }
  return XML_TRUE;
}


Best Regards,

mj

----------------------------------------------------------------------

Comment By: Karl Waclawek (kwaclaw)
Date: 2006-10-26 09:07

Message:
Logged In: YES 
user_id=290026

> ### ABOVE REALLOC may return different memory block so
> following tag->buf access may become invalid

> "### BUG -->" if (tag->name.str == (XML_Char *)tag->buf)
> tag->name.str = (XML_Char *)temp;

There is no dereferencing of the tag->buf pointer, just
a pointer comparison, which can be done on any pointer,
valid or invalid.

Do you actually have a test case where you can reproduce
an invalid memory access at this point int the code?


----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=110127&aid=1584898&group_id=10127