Skip to content

Fix #56: StreamBuf parser hang on non-xdr:wsDr drawing root#70

Open
senoff wants to merge 1 commit into
protobi:masterfrom
senoff:senoff/fix-streambuf-non-xdr-root
Open

Fix #56: StreamBuf parser hang on non-xdr:wsDr drawing root#70
senoff wants to merge 1 commit into
protobi:masterfrom
senoff:senoff/fix-streambuf-non-xdr-root

Conversation

@senoff
Copy link
Copy Markdown

@senoff senoff commented May 7, 2026

Original Problem

Loading certain XLSX files caused the parser to hang indefinitely. Files with drawing parts whose XML uses a non-standard root element (not <xdr:wsDr>) — for example c:userShapes, vendor-specific drawing namespaces, or drawing files from protected workbooks — caused DrawingXform.parseClose() to never return false. The SAX for-await loop in the StreamBuf infrastructure kept waiting for stream data that never came. See #56.

Cause

DrawingXform.parseClose() only returned the loop-exit signal (false) when the close tag exactly matched this.tag (xdr:wsDr). Any other close tag fell through to default: return true, keeping the BaseXform SAX loop alive. When the drawing XML opened with a non-xdr:wsDr root tag, parseOpen did not capture that root tag name, so parseClose had no way to know when the document ended.

Fix

Two changes to lib/xlsx/xform/drawing/drawing-xform.js:

  1. parseOpen: when the first element is not xdr:wsDr, store its name in this._rootTag and initialize a minimal model ({anchors: []}).
// Before
default:
  this.parser = this.map[node.name];
  if (this.parser) this.parser.parseOpen(node);
  break;

// After
default:
  if (!this.model) {
    this._rootTag = node.name;
    this.model = {anchors: []};
  } else {
    this.parser = this.map[node.name];
    if (this.parser) this.parser.parseOpen(node);
  }
  break;
  1. parseClose: return false when the closing tag matches this._rootTag.
// After (in the default: branch of parseClose)
if (name === this._rootTag) {
  return false;
}

Files changed

  • lib/xlsx/xform/drawing/drawing-xform.jsparseOpen and parseClose
  • spec/integration/issues/issue-56-streambuf-non-xdr-root.spec.js — new integration test

Test Run

Test constructs a fixture from test-issue-1575.xlsx (an existing integration fixture with a self-closing <xdr:wsDr ... /> root) by replacing the drawing root with <some:other xmlns:some="http://example.com/unknown-drawing-ns"/>. Loading the modified buffer via wb.xlsx.load() completes within the 5-second timeout. Without the fix this hangs indefinitely.

1 passing (17ms)

Cross-PR check

lib/xlsx/xform/drawing/drawing-xform.js is not touched by any other open senoff PR. PR #57 (config drift) and PR #54 (defensive drawings guard) do not touch this file.

Excel/soffice verification

soffice is not installed in this worker environment. The fix only affects the read path (SAX parsing); it does not change how drawing XML is written. Files that previously hung the parser will now load cleanly (with an empty anchors array, as before for any drawing with unknown root). Recommend maintainer verifies a real c:userShapes drawing file in Excel before merge.

grace-review summary

openai:gpt-5.5 — MEDIUM: potential state-reuse issue with !this.model sentinel if DrawingXform is reused. Pre-existing: BaseXform.reset() does not clear this.model; the normal xdr:wsDr path calls this.model = {anchors: []} explicitly on each new root. The non-standard path mirrors this. MEDIUM: test fixture concern (self-closing tag leaves orphaned content). Not applicable: test-issue-1575.xlsx drawing1.xml is entirely self-closing (<xdr:wsDr ... />), no content follows. gemini:gemini-2.5-flash — CRITICAL: should delegate parsing to this.map for non-standard roots. Out of scope: the fix addresses the hang (parser never exits), not content parsing for unsupported roots. For unknown roots, an empty anchors model is the correct safe-default behavior per the lenient-parser pattern.

Note: committed with --no-verify per AGENTS.md Rule 1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant