Skip to content

fix: dispose backing FileStream on ExcelHandler/PowerPointHandler constructor failure - #224

Open
Marnie0415 wants to merge 1 commit into
iOfficeAI:mainfrom
Marnie0415:fix/handler-constructor-filestream-leak
Open

fix: dispose backing FileStream on ExcelHandler/PowerPointHandler constructor failure#224
Marnie0415 wants to merge 1 commit into
iOfficeAI:mainfrom
Marnie0415:fix/handler-constructor-filestream-leak

Conversation

@Marnie0415

@Marnie0415 Marnie0415 commented Jul 14, 2026

Copy link
Copy Markdown

Summary

  • ExcelHandler constructor only catches OpenXmlPackageException; other failures (IOException, UnauthorizedAccessException) leave _backingStream undisposed
  • PowerPointHandler constructor has no try/catch — any failure from PresentationDocument.Open leaks the FileStream
  • WordHandler already has the correct pattern: try/catch that disposes both _doc and _backingStream on failure

Root cause

When opening a corrupted or locked Office file, the SDK's Open method may throw exceptions beyond OpenXmlPackageException. The factory's repair-and-retry paths (FixXmlEncoding / StripDanglingPackageRels) reopen the file for in-place fixes — but the leaked FileStream from the first attempt causes "file is being used by another process".

Fix

Apply the same cleanup pattern as WordHandler (line 342-350):

  • ExcelHandler: catch-all block disposes _doc, _filteredPackageStream, and _backingStream
  • PowerPointHandler: wrap PresentationDocument.Open + InitShapeIdCounter in try/catch; dispose _doc and _backingStream

Both handlers now use catch-all (matching WordHandler) so the original exception propagates directly — the factory's IsEncodingException / IsDanglingPartException checks walk the exception chain and continue to work correctly.

Verification

# 1. Create a truncated xlsx file
python -c "open('broken.xlsx','wb').write(b'PK\x03\x04broken')"

# 2. Run command — should get clean error, no leaked handle
officecli get broken.xlsx / --json
# Before fix: file handle leaked, subsequent ops may fail with "file is being used"
# After fix: clean error reported, file handle released

# 3. Verify no lingering handle (Linux)
lsof | grep broken.xlsx  # should produce no output

# 4. Verify normal files are unaffected
officecli create test.xlsx
officecli add test.xlsx /Sheet1 cell --prop value=hello
officecli get test.xlsx /Sheet1/A1

Testing

  • Verified on Windows 11 — corrupted file reports error cleanly, no handle leak
  • Verified normal xlsx/pptx open/edit/save cycle unaffected
  • Verified factory retry paths (encoding fix, dangling rel strip) still work
  • Linux/macOS — maintainer may wish to verify

Breaking Changes

None. The exception propagation path changes slightly (original exception rethrown directly instead of wrapped in InvalidOperationException), but the factory's catch chain handles both forms correctly. End users see the same corrupt_file CLI error.

Notes

  • Changes follow the existing WordHandler pattern exactly (~27 lines across 2 files)
  • No test project exists in the repo — CONTRIBUTING.md states tests are handled by maintainer post-merge

…structor failure

ExcelHandler and PowerPointHandler constructors create a backing
FileStream but only catch OpenXmlPackageException (ExcelHandler) or
no exceptions at all (PowerPointHandler). If PresentationDocument.Open
or SpreadsheetDocument.Open fails for other reasons (IOException,
UnauthorizedAccessException), the FileStream is never disposed — the
file handle leaks until process exit.

WordHandler already has the correct pattern: try/catch that disposes
both _doc and _backingStream on failure. Apply the same cleanup to
ExcelHandler (also disposing _filteredPackageStream for the bloat-
filter path) and PowerPointHandler.

Both handlers now use catch-all (matching WordHandler) so the original
exception propagates directly — the factory's repair-and-retry paths
(IsEncodingException, IsDanglingPartException) walk the exception
chain and continue to work correctly.
@Marnie0415
Marnie0415 force-pushed the fix/handler-constructor-filestream-leak branch from 52bdc25 to cf4162e Compare July 14, 2026 23:08
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