[Expat-bugs] [ expat-Bugs-620343 ] segfault: bad API/callbackinteraction

Karl Waclawek karl at waclawek.net
Tue Jan 21 11:12:46 EST 2003


Here is the follow-up I promised:

Let's say we try to fix the problem that calling the nameSpaceDeclHandlers
depends on startElementHandler being set, since this means: storeAtts will
be called with tagNamePtr and bindingsPtr not NULL, which will cause
namespace declarations to be processed.

Here is an attempt (abbreviated):

    case XML_TOK_START_TAG_WITH_ATTS:
      haveElmHandlers = startElementHandler || startNamespaceDeclHandler
        || endNamespaceDeclHandler;
      if (!haveElmHandlers) {
        enum XML_Error result = storeAtts(parser, enc, s, 0, 0);
        if (result)
          return result;
      }
      /* fall through */
    case XML_TOK_START_TAG_NO_ATTS:
      {
        ... <lots of stuff cut>
        tag->name.str = (XML_Char *)tag->buf;
        *toPtr = XML_T('\0');
        if (haveElmHandlers) {
          result = storeAtts(parser, enc, s, &(tag->name), &(tag->bindings));
          if (result)
            return result;
          if (startElementHandler)
            startElementHandler(handlerArg, tag->name.str,
                                (const XML_Char **)atts);
        }
        else if (defaultHandler)
          reportDefault(parser, enc, s, next);
        poolClear(&tempPool);
        break;
      }

What if in the startNamespaceDeclHandler (called from storeAtts) we reset the
startElementHandler or the endNamespaceDeclHandler?
What happens in those cases with calling the defaultHandler?

Maybe the defaultHandler is only a substitute for the startElementHandler here?
So then it should look like this:

    case XML_TOK_START_TAG_NO_ATTS:
      {
        ... <lots of stuff cut>
        tag->name.str = (XML_Char *)tag->buf;
        *toPtr = XML_T('\0');
        if (haveElmHandlers) {
          result = storeAtts(parser, enc, s, &(tag->name), &(tag->bindings));
          if (result)
            return result;
          if (startElementHandler)
            startElementHandler(handlerArg, tag->name.str,
                                (const XML_Char **)atts);
          else if (defaultHandler)
            reportDefault(parser, enc, s, next);
        }
        else if (defaultHandler)
          reportDefault(parser, enc, s, next);
        poolClear(&tempPool);
        break;
      }

Any opinions?

Karl



----- Original Message ----- 
From: "SourceForge.net" <noreply at sourceforge.net>
To: <noreply at sourceforge.net>
Sent: Tuesday, January 21, 2003 10:55 AM
Subject: [Expat-bugs] [ expat-Bugs-620343 ] segfault: bad API/callbackinteraction


> Bugs item #620343, was opened at 2002-10-08 12:50
> You can respond by visiting: 
> https://sourceforge.net/tracker/?func=detail&atid=110127&aid=620343&group_id=10127
> 
> Category: None
> Group: None
> Status: Open
> Resolution: Fixed
> Priority: 5
> Submitted By: Fred L. Drake, Jr. (fdrake)
> Assigned to: Fred L. Drake, Jr. (fdrake)
> Summary: segfault: bad API/callback interaction
> 
> Initial Comment:
> There is a bad interaction between the call to the
> start element handler and the Expat API:  After the
> presence of the callback is checked, the addBinding()
> function can call XML_SetStartElementHandler(parser,
> NULL), after which doContent() will still attempt to
> call the start element handler.  This results in a call
> to the NULL address, resulting in a memory fault.
> 
> This is unlikely to be an issue for applications
> written entirely in C, but can reasonably happen when a
> wrapper library clears callbacks when an error
> condition or exception has been detected.
> 
> I've attached a patch and a regression test for this case.
> 
> 
> ----------------------------------------------------------------------
> 
> >Comment By: Karl Waclawek (kwaclaw)
> Date: 2003-01-21 10:55
> 
> Message:
> Logged In: YES 
> user_id=290026
> 
> I had another look and this looks trickier than I thought.
> I suggest we add documentation that states that
> the start/endNamespaceDeclHandlers will not be called
> when the startElementHandler is not set.
> 
> I'll post more details in a follow-up.
> 
> ----------------------------------------------------------------------
> 
> Comment By: Karl Waclawek (kwaclaw)
> Date: 2002-10-08 13:40
> 
> Message:
> Logged In: YES 
> user_id=290026
> 
> I am not *that* eager <g>.
> You take care of it.
> 
> Thanks!
> 
> ----------------------------------------------------------------------
> 
> Comment By: Fred L. Drake, Jr. (fdrake)
> Date: 2002-10-08 13:36
> 
> Message:
> Logged In: YES 
> user_id=3066
> 
> Original patch checked in as lib/xmlparse.c 1.92 and
> tests/runtests.c 1.36, before I saw Karl's comments.
> 
> Karl:  Yes, we should probably fix things so the namespace
> decl handlers can be set independently, as the API suggests.
>  I can take care of this, unless you beat me to it.  ;-)
> 
> Don't know that it warrants a separate issue report, so I'll
> leave this one as fixed but open for now.
> 
> ----------------------------------------------------------------------
> 
> Comment By: Karl Waclawek (kwaclaw)
> Date: 2002-10-08 13:21
> 
> Message:
> Logged In: YES 
> user_id=290026
> 
> Yes, verified. To be precise, addBinding() can call back
> to startNamespaceDeclHandler() which then allows
> the application to clear the startElementHandler.
> 
> One comment: storeAtts() is called like this
> 
>   if (startElementHandler) {
>     result = storeAtts(parser, ...);
>     ...
>   }
> 
> presumably because the attributes should only be
> stored if needed for the startElementHandler.
> However, storeAtts does double duty by also
> processing namespace declarations and calling
> startNamespaceDeclHandler().
> 
> So, if that handler is set, should then not storeAtts()
> be called like this:
> 
>   if (startElementHandler || startNamespaceDeclHandler)
>   {
>     result = storeAtts(parser, ...);
>     ...
>   }
> 
> And, if true, one should then apply this logic to the
> other occasions when storeAtts() is called.
> 
> Or does it not make sense to set 
> startNamespaceDeclHandler when
> startElementHandler is cleared?
> 
> 
> 
> ----------------------------------------------------------------------
> 
> You can respond by visiting: 
> https://sourceforge.net/tracker/?func=detail&atid=110127&aid=620343&group_id=10127
> 
> _______________________________________________
> Expat-bugs mailing list
> Expat-bugs at libexpat.org
> http://mail.libexpat.org/mailman/listinfo/expat-bugs
>



More information about the Expat-bugs mailing list