TIKA-4744 (#2850)

Author: tballison

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-apple-module/src/main/java/org/apache/tika/parser/iwork/KeynoteContentHandler.java

@@ -142,6 +142,12 @@ public void endElement(String uri, String localName, String qName) throws SAXExc
         } else if (inMetadata && "key:authors".equals(qName)) {
             inMetaDataAuthors = false;
         } else if (inSlide && "sf:tabular-model".equals(qName)) {
+            // If the final row has fewer cells than numberOfColumns,
+            // parseTableData never reaches its </tr> emit. Close any open
+            // row before </table> so the SAX stream stays balanced.
+            if (currentColumn != null && currentColumn != 0) {
+                xhtml.endElement("tr");
+            }
             xhtml.endElement("table");
             tableId = null;
             numberOfColumns = null;

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-apple-module/src/test/java/org/apache/tika/parser/iwork/KeynoteContentHandlerTest.java

@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.tika.parser.iwork;
+
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import org.junit.jupiter.api.Test;
+import org.xml.sax.helpers.AttributesImpl;
+
+import org.apache.tika.metadata.Metadata;
+import org.apache.tika.parser.ParseContext;
+import org.apache.tika.sax.StrictXHTMLValidator;
+import org.apache.tika.sax.ToXMLContentHandler;
+import org.apache.tika.sax.XHTMLContentHandler;
+
+public class KeynoteContentHandlerTest {
+
+    /**
+     * TIKA-4744: Keynote tables whose final row contains fewer cells than
+     * numberOfColumns left {@code <tr>} open when {@code </sf:tabular-model>}
+     * fired {@code </table>}. Drive the handler with a synthetic 3-column
+     * table whose final row has only 2 cells and assert the XHTML stays
+     * balanced through endDocument.
+     */
+    @Test
+    public void testIncompleteFinalRowClosesTr() throws Exception {
+        Metadata md = new Metadata();
+        ToXMLContentHandler xml = new ToXMLContentHandler();
+        XHTMLContentHandler xhtml = new XHTMLContentHandler(
+                new StrictXHTMLValidator(xml), md, new ParseContext());
+        KeynoteContentHandler h = new KeynoteContentHandler(xhtml, md);
+
+        xhtml.startDocument();
+
+        // <key:slide>
+        h.startElement("", "slide", "key:slide", new AttributesImpl());
+        // <sf:tabular-model sfa:ID="t1">
+        AttributesImpl tabAttrs = new AttributesImpl();
+        tabAttrs.addAttribute("", "sfa:ID", "sfa:ID", "CDATA", "t1");
+        h.startElement("", "tabular-model", "sf:tabular-model", tabAttrs);
+        // <sf:columns sf:count="3">
+        AttributesImpl colAttrs = new AttributesImpl();
+        colAttrs.addAttribute("", "sf:count", "sf:count", "CDATA", "3");
+        h.startElement("", "columns", "sf:columns", colAttrs);
+        h.endElement("", "columns", "sf:columns");
+        // Two cells (less than 3) -> incomplete final row.
+        AttributesImpl cell = new AttributesImpl();
+        cell.addAttribute("", "sfa:s", "sfa:s", "CDATA", "A");
+        h.startElement("", "ct", "sf:ct", cell);
+        h.endElement("", "ct", "sf:ct");
+        cell = new AttributesImpl();
+        cell.addAttribute("", "sfa:s", "sfa:s", "CDATA", "B");
+        h.startElement("", "ct", "sf:ct", cell);
+        h.endElement("", "ct", "sf:ct");
+        // </sf:tabular-model> -- without the fix this emits </table> while
+        // <tr> is still topmost and StrictXHTMLValidator throws.
+        h.endElement("", "tabular-model", "sf:tabular-model");
+        // </key:slide>
+        h.endElement("", "slide", "key:slide");
+
+        xhtml.endDocument();
+
+        String out = xml.toString();
+        // Sanity: the cells emitted, the row closed, and the table closed.
+        assertTrue(out.contains("<td>A</td>"), "expected td A; got: " + out);
+        assertTrue(out.contains("<td>B</td>"), "expected td B; got: " + out);
+        assertTrue(out.contains("</tr>"), "expected </tr>; got: " + out);
+        assertTrue(out.contains("</table>"), "expected </table>; got: " + out);
+    }
+}

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/parser/microsoft/ooxml/FormattingTagManager.java

@@ -114,14 +114,16 @@ void applyFormatting(RunProperties runProperties) throws SAXException {
         }
 
         if (runProperties.isBold() != isBold) {
-            if (isStrikeThrough) {
-                xhtml.endElement("s");
-                isStrikeThrough = false;
-            }
+            // Close inner tags before flipping <b>. Nesting is <b><i><s><u>
+            // (outermost to innermost), so close innermost first: u, s, i.
             if (isUnderline) {
                 xhtml.endElement("u");
                 isUnderline = false;
             }
+            if (isStrikeThrough) {
+                xhtml.endElement("s");
+                isStrikeThrough = false;
+            }
             if (isItalics) {
                 xhtml.endElement("i");
                 isItalics = false;
@@ -135,14 +137,15 @@ void applyFormatting(RunProperties runProperties) throws SAXException {
         }
 
         if (runProperties.isItalics() != isItalics) {
-            if (isStrikeThrough) {
-                xhtml.endElement("s");
-                isStrikeThrough = false;
-            }
+            // Close inner tags before flipping <i>: u then s (u is innermost).
             if (isUnderline) {
                 xhtml.endElement("u");
                 isUnderline = false;
             }
+            if (isStrikeThrough) {
+                xhtml.endElement("s");
+                isStrikeThrough = false;
+            }
             if (runProperties.isItalics()) {
                 xhtml.startElement("i");
             } else {

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/parser/microsoft/ooxml/OOXMLTikaBodyPartHandler.java

@@ -265,7 +265,16 @@ private void popExpected(String tag) {
 
     @Override
     public void startTable() throws SAXException {
-
+        // A <w:tbl> can appear nested inside an outer <w:p> -- corrupt-ish but
+        // present in the corpus (e.g., <w:p><w:r>...<wps:txbx><w:txbxContent>
+        // <w:tbl>...). At that point a run-level <b>/<i>/<u>/<s>/<a> may be on
+        // the SAX stack just above where the <table> is about to land. When a
+        // later paragraph inside a cell ends, formattingTags.closeAll() tries
+        // to emit </b> for the outer-paragraph state, but <td> is topmost --
+        // strict validator rejects the mismatch. Close pending formatting now
+        // so the table opens at a clean layer and the outer style is forgotten.
+        // Mirrors startSDT()'s same-shape guard.
+        formattingTags.closeAll();
         xhtml.startElement("table");
         openStructuralTags.push("table");
         tableDepth++;
@@ -377,14 +386,21 @@ private void inlineNoteContent(byte[] xml, String cssClass) throws SAXException
         // from the footnote/endnote parts (needed for picture resolution)
         Map<String, String> noteRelationships = inlinePartMap.getLinkedRelationships();
         xhtml.startElement("div", "class", cssClass);
+        // Track the inner handler so we can call its closeAnyPending() if
+        // the inline-note parseSAX aborts mid-element. Without the drain
+        // the surrounding </div> mismatches whatever the inner handler
+        // left on the SAX stack (<p>/<td>/etc.) and StrictXHTMLValidator
+        // propagates a misleading error.
+        OOXMLTikaBodyPartHandler innerHandler = new OOXMLTikaBodyPartHandler(xhtml);
         try {
             XMLReaderUtils.parseSAX(new ByteArrayInputStream(xml),
                     new EmbeddedContentHandler(
                             new OOXMLWordAndPowerPointTextHandler(
-                                    new OOXMLTikaBodyPartHandler(xhtml),
+                                    innerHandler,
                                     noteRelationships)),
                     parseContext);
-        } catch (TikaException | IOException e) {
+        } catch (TikaException | IOException | SAXException e) {
+            innerHandler.closeAnyPending();
             xhtml.characters("[" + cssClass + " parse error]");
         }
         xhtml.endElement("div");

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/parser/microsoft/ooxml/OOXMLWordAndPowerPointTextHandler.java

@@ -146,7 +146,11 @@ public class OOXMLWordAndPowerPointTextHandler extends DefaultHandler {
     //have we signaled the start of a p?
     //pPr can happen multiple times within a p
     //<p><pPr/><r><t>text</t></r><pPr></p>
-    private boolean pStarted = false;
+    //
+    //Stack rather than a single boolean: nested <w:p> (e.g., inside
+    //<w:txbxContent>) must not clobber the outer paragraph's "started" marker,
+    //or the outer </w:p> will skip its endParagraph and leave <p> open.
+    private final java.util.Deque<Boolean> pStartedStack = new java.util.ArrayDeque<>();
     //alternate content can be embedded in itself.
     //need to track depth.
     //preferACChoice controls which branch is processed:
@@ -238,6 +242,7 @@ public void startElement(String uri, String localName, String qName, Attributes
         } else if (lastStartElementWasP) {
             // First child of <p> is not pPr — start paragraph immediately with defaults.
             bodyContentsHandler.startParagraph(currPProperties);
+            markCurrentParagraphStarted();
         }
 
         lastStartElementWasP = false;
@@ -271,16 +276,15 @@ public void startElement(String uri, String localName, String qName, Attributes
             runBuffer.append(TAB_CHAR);
         } else if (P.equals(localName)) {
             lastStartElementWasP = true;
-            // Each <w:p> needs its own pStarted lifecycle. Without this,
-            // a nested <w:p> (e.g., inside <wps:txbx>/<w:txbxContent>) would
-            // inherit the outer paragraph's pStarted=true, suppress its own
-            // startParagraph in the </w:pPr> branch, then fire its
-            // endParagraph on </w:p> -- producing an unbalanced start/end
-            // count that desyncs the XHTML <p>/<p> stream.
-            pStarted = false;
+            // Push a fresh frame for this <w:p>. A nested <w:p> (e.g., inside
+            // <w:txbxContent>) must not share the outer paragraph's started-flag,
+            // or the outer </w:p>'s endParagraph either fires twice (older bug)
+            // or gets skipped (after the pStarted guard fix), and the XHTML
+            // <p>/<p> stream desyncs either way.
+            pStartedStack.push(Boolean.FALSE);
         } else if (B.equals(localName)) { //TODO: add bCs
             if (inR && inRPr) {
-                currRunProperties.setBold(true);
+                currRunProperties.setBold(getOnOff(atts, true));
             }
         } else if (TC.equals(localName)) {
             bodyContentsHandler.startTableCell();
@@ -290,11 +294,11 @@ public void startElement(String uri, String localName, String qName, Attributes
         } else if (I.equals(localName)) { //TODO: add iCs
             //rprs don't have to be inR; ignore those that aren't
             if (inR && inRPr) {
-                currRunProperties.setItalics(true);
+                currRunProperties.setItalics(getOnOff(atts, true));
             }
         } else if (STRIKE.equals(localName)) {
             if (inR && inRPr) {
-                currRunProperties.setStrike(true);
+                currRunProperties.setStrike(getOnOff(atts, true));
             }
         } else if (U.equals(localName)) {
             if (inR && inRPr) {
@@ -491,6 +495,33 @@ private String getStringVal(Attributes atts) {
         return "";
     }
 
+    /**
+     * Reads a {@code ST_OnOff} {@code w:val} attribute: {@code "0"}/{@code "false"}/
+     * {@code "off"} are off, anything else (including absent) follows the supplied
+     * default. The toggle elements (&lt;w:b/&gt;, &lt;w:i/&gt;, &lt;w:strike/&gt;)
+     * default to on when {@code w:val} is absent, but must respect an explicit
+     * {@code w:val="0"} that turns the toggle off (overriding a style-inherited on).
+     */
+    private boolean getOnOff(Attributes atts, boolean defaultValue) {
+        String v = atts.getValue(W_NS, VAL);
+        if (v == null) {
+            return defaultValue;
+        }
+        return !("0".equals(v) || "false".equals(v) || "off".equals(v));
+    }
+
+    private boolean isCurrentParagraphStarted() {
+        Boolean top = pStartedStack.peek();
+        return top != null && top;
+    }
+
+    private void markCurrentParagraphStarted() {
+        if (!pStartedStack.isEmpty()) {
+            pStartedStack.pop();
+        }
+        pStartedStack.push(Boolean.TRUE);
+    }
+
     private int getIntVal(Attributes atts) {
         String valString = atts.getValue(W_NS, VAL);
         if (valString != null) {
@@ -531,9 +562,9 @@ public void endElement(String uri, String localName, String qName) throws SAXExc
         } else if (PPR.equals(localName) && inParagraphLevelPPr) {
             // Only process as paragraph properties if this pPr was a direct child of <p>.
             // pPr inside other elements (e.g., <a:fld> fields) must be ignored.
-            if (!pStarted) {
+            if (!isCurrentParagraphStarted()) {
                 bodyContentsHandler.startParagraph(currPProperties);
-                pStarted = true;
+                markCurrentParagraphStarted();
             }
             currPProperties.reset();
             inParagraphLevelPPr = false;
@@ -544,8 +575,19 @@ public void endElement(String uri, String localName, String qName) throws SAXExc
                 bodyContentsHandler.run(currRunProperties, runBuffer.toString());
                 runBuffer.setLength(0);
             }
-            pStarted = false;
-            bodyContentsHandler.endParagraph();
+            // Only fire endParagraph if startParagraph was actually called for this <w:p>.
+            // A self-closing <w:p/> (e.g., inside <w:txbxContent>) has no children, so
+            // neither the </pPr> branch nor the lastStartElementWasP branch fires
+            // startParagraph -- but endElement(p) still runs. Without this guard the
+            // body handler's pDepth counter desyncs and the outer paragraph's </p> gets
+            // emitted prematurely, leaving the XHTML stack mismatched at endDocument.
+            boolean started = pStartedStack.isEmpty() ? false : pStartedStack.pop();
+            if (started) {
+                bodyContentsHandler.endParagraph();
+            }
+            // Clear the "first child of p" trigger so the next outer-level startElement
+            // doesn't spuriously fire startParagraph for this already-closed <w:p/>.
+            lastStartElementWasP = false;
         } else if (TC.equals(localName)) {
             bodyContentsHandler.endTableCell();
         } else if (TR.equals(localName)) {

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/parser/microsoft/ooxml/XSSFExcelExtractorDecorator.java

@@ -239,6 +239,13 @@ protected void buildXHTML(XHTMLContentHandler xhtml)
                     // the </tbody></table></div> emitted below land in the
                     // right place.
                     sheetExtractor.closeAnyPending();
+                } catch (IOException e) {
+                    // Truncated stream — same risk: partial <tr>/<td> still
+                    // open. Close them so the surrounding </tbody></table>
+                    // stays balanced, record the failure, and keep going.
+                    metadata.add(TikaCoreProperties.TIKA_META_EXCEPTION_WARNING,
+                            ExceptionUtils.getStackTrace(e));
+                    sheetExtractor.closeAnyPending();
                 }
                 try {
                     getThreadedComments(container, sheetPart, xhtml);

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/parser/microsoft/rtf/RTFEmbObjHandler.java

@@ -35,6 +35,7 @@
 import org.apache.tika.metadata.TikaCoreProperties;
 import org.apache.tika.parser.ParseContext;
 import org.apache.tika.sax.EmbeddedContentHandler;
+import org.apache.tika.sax.XHTMLBalancingHandler;
 
 /**
  * This class buffers data from embedded objects and pictures.
@@ -235,12 +236,24 @@ private void extractObj(byte[] bytes, ContentHandler handler, Metadata metadata)
                     }
                     metadata.set(TikaCoreProperties.RESOURCE_NAME_EXTENSION_INFERRED, true);
                 }
+                // Wrap the outer handler in a balancing handler so that if
+                // the embedded parser aborts mid-element (leaving <p>/<b>/etc
+                // open on the wire), we can drain those before propagating.
+                // Without this, the outer RTF parser's subsequent
+                // </b>/</p>/</body> sequence trips StrictXHTMLValidator on
+                // unbalanced nesting and surfaces as a misleading error at
+                // endDocument. Same shape as the SXWPF / Epub catch arms.
+                XHTMLBalancingHandler balancer = new XHTMLBalancingHandler(handler);
                 try {
                     embeddedDocumentUtil
-                            .parseEmbedded(tis, new EmbeddedContentHandler(handler), metadata,
+                            .parseEmbedded(tis, new EmbeddedContentHandler(balancer), metadata,
                                     true);
                 } catch (IOException e) {
+                    balancer.drainOpenElements();
                     EmbeddedDocumentUtil.recordEmbeddedStreamException(e, metadata);
+                } catch (SAXException e) {
+                    balancer.drainOpenElements();
+                    EmbeddedDocumentUtil.recordException(e, metadata);
                 }
             }
         }

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/parser/microsoft/rtf/RTFParser.java

@@ -35,6 +35,7 @@
 import org.apache.tika.mime.MediaType;
 import org.apache.tika.parser.ParseContext;
 import org.apache.tika.parser.Parser;
+import org.apache.tika.sax.XHTMLBalancingHandler;
 import org.apache.tika.sax.XHTMLContentHandler;
 
 /**
@@ -90,11 +91,26 @@ public void parse(TikaInputStream tis, ContentHandler handler, Metadata metadata
         TaggedInputStream tagged = new TaggedInputStream(tis);
         XHTMLContentHandler xhtml = new XHTMLContentHandler(handler, metadata, context);
         xhtml.startDocument();
+        // Wrap xhtml in a balancing handler so the finally's endDocument
+        // doesn't fire on an unbalanced stack if the RTF state machine
+        // emits inconsistent SAX events (e.g., </b> while <p> is topmost
+        // due to state-vs-stack drift on certain corpus files). Without
+        // this, StrictXHTMLValidator's </body> vs <p>/<b> at endDocument
+        // masks the real well-formedness error from inside extract().
+        XHTMLBalancingHandler balancer = new XHTMLBalancingHandler(xhtml);
         try {
-            parseInline(tis, xhtml, metadata, context);
+            parseInline(tis, balancer, metadata, context);
         } catch (IOException e) {
             tagged.throwIfCauseOf(e);
+            balancer.drainOpenElements();
             throw new TikaException("Error parsing an RTF document", e);
+        } catch (SAXException | TikaException e) {
+            // Drain on any exception escaping parseInline. parseInline can
+            // throw TikaException too (memory limits, embedded extraction,
+            // etc.); without the drain, the finally's xhtml.endDocument()
+            // throws on the unbalanced stack and masks the real error.
+            balancer.drainOpenElements();
+            throw e;
         } finally {
             xhtml.endDocument();
         }