XOM API for Processing XML with Java

[David Bullock <db AT dawnbreaks.net>]

On Mon, May 19, 2014 at 12:46 AM, Elliotte Rusty Harold
<elharo AT ibiblio.org>wrote:


> In particular, rather than causing an out of memory error, it seems to have
> hung Xerces, possibly pushing it into an infinite loop.
>

That'll be tricky to recover from, even if you're running it in a separate
thread.  You'll need to first try to 'interrupt' it and wait a bit longer,
hoping the Xerces code is in a state where it can honour the interruption
status (which seems unlikely if its got itself in an unguarded loop, which
is a logic error to begin with).

If Xerces won't respond to interruption when in its failure state, I think
the only right thing to do is to to blacklist any faulty Xerces
implementations you know of and throw a
CannotComplyUntilYouUpgradeXercesException as early as possible when the
user attempts to enable DoS protection.  Calling Thread.destroy() might
seem tempting, but without perfect knowledge that (a given implementation
of) Xerces hasn't allocated any threads/file-handles/etc, then you are only
moving the problem to a future resource exhaustion.

Speaking of resource exhaustion, if you started a thread, you must also
stop it.  Including when your thread is interrupted.

In regard to your library being used in managed frameworks, it is OK to
start a 'sub thread' which has terminated by the time your 'blocking'
method call returns.  But you must be able to make sure you can and do stop
the threads you start.  (Notice that starting a 'thread per request' *may*
not have happy performance properties).



> So here's my thought. In addition to setting a maximum memory size on
> documents, I will set a maximum execution *time*. I.e. I'll move the parse
> into a separate thread and abort it if it exceeds a configurable amount of
> time. The code is something like this:
>
>         Document result = null;
>         try {
>           ParseJob job = new ParseJob(in);
>             Thread t = new Thread(job);
>             t.start();
>             t.join(60000);
>             if (job.getProblem() != null) {
>               throw job.getProblem();
>             }
>             result = handler.getDocument();
>         }
>

The problem with this code is that you're not cleaning up the thread you
created under the circumstances where:

* the timed wait via t.join() actually times out without the sub-thread
having stopped yet
* your own thread gets interrupted

This is better:

public Document ParseWithDosProtection() throws InterruptedException {

ParseJob job = new ParseJob(in);
Thread t = new Thread(job);

t.start();

try {
   t.join(6000);
   if (thead.isAlive()) {
      t.interrupt();
      // and because we care ...
         t.join(1000);
         if (thread.isAlive()) {
             throw new ArghException("Stupid Xerces! Your system is in an
unknown state, sorry. Xerces might release locks/resources/CPU, or it might
not.  We felt it was more important to get back to you in a reasonable
time, than wait indefinitely to know about Xerces' real status.");
         }
      return handler.getDocument();
    }
} catch (InterruptedException e) {  // happens if someone interrupts US
before t.join(6000) or t.join(1000) return naturally
    t.interrupt();
    // more important to follow the interruption protocol than communicate
how stupid Xerces is
    // but see listing 3 of
http://www.ibm.com/developerworks/library/j-jtp05236/ if you think callers
will want to know
    throw e;
}

} // end method



>     private class ParseJob implements Runnable {
>

Seems fine.


I'd be tempted to implement the protections by streaming the input through
a simple automaton which knew how to track entity expansion and usage,
*before* it reaches the proper parser.  Presumably, you can asses from the
doctype information alone the 'degree' of combinatorial expansion it
implies, without the trouble of actually expanding it?

cheers,
David.