Bug47950 -- make stream/directory name lookup in OLE2 case insensitive #477

tballison · 2023-06-21T15:59:02Z

This is a first draft at making stream/directory name look up case insensitive for OLE2 containers.

…ve documentation slightly

pjfanning · 2023-06-21T16:56:13Z

poi/src/main/java/org/apache/poi/hssf/usermodel/HSSFWorkbook.java

@@ -297,11 +298,15 @@ public static String getWorkbookDirEntryName(DirectoryNode directory) {
                    "It must be decrypted before use by XSSF, it cannot be used by HSSF");
        }

-        // check for previous version of file format
-        if (directory.hasEntry(OLD_WORKBOOK_DIR_ENTRY_NAME)) {


Not a big deal if this is too hard but is there any chance that hasEntry could remain case sensitive for backward compatibility reasons? And a new caseInsensitiveHasEntry added?

If leaving hasEntry case sensitive is too hard, could you could not using directory.getEntryNames().contains and adding a caseSensitiveHasEntry instead?

Y, I don't like the backwards compatibility break that this proposed change brings about. If we don't do this, though, then we'll have to change hasEntry(name) and getEntry(name) to the case insensitive options throughout our codebase. I can do that if desired.

Yep, I'll add that now.

poi/src/main/java/org/apache/poi/hssf/usermodel/HSSFWorkbook.java

tballison · 2023-06-21T17:11:14Z

I look forward to running the regression tests after this change. 😄 Let me know what else you find. I haven't worked with POI in a while, and this is a major change in behavior.

tballison · 2023-06-22T15:02:21Z

Results show no significant changes in ~400k msoffice files (not all ole2): https://corpora.tika.apache.org/base/reports/tika-2.8.1-msoffice-reports.tgz

There is a major difference in handling "true"/"false" "1/0" in xlsx(?only?) between the versions of POI, but this change is unrelated.

pjfanning · 2023-06-22T17:00:43Z

@tballison could you raise an issue for the xssf boolean issue with and xlsx attached that makes it easier to debug?

With this PR, I'm still a little concerned about changing the behaviour of hasEntry/getEntry. I suppose that we could agree that the next release of POI has to be a v5.3.0 instead of v5.2.4 because of the behaviour change.

tballison · 2023-06-22T19:57:38Z

I'm not sure the boolean issue is wrong, just different. When I opened a couple of the files, there was nothing in the cells. Will look deeper later.

I'm also concerned about changing the behavior. We could go to 5.3.0, or I could have a major refactoring where I modify all the internal hasEntry and getEntry to hasEntryCaseInsensitive and getEntryCaseInsensitive. If Intellij is accurate, there are ~230 uses of getEntry and ~90 hasEntry.

I'm willing to make that change, but it will be a large set of changes -- TBF, a bunch are in unit tests. Let me know what you think.

The other thing I shouldn't have done was change the values of InternalWorkbook#WORKBOOK_DIR_ENTRY_NAMES. I should have left that as is and made my own mods on a new field. I'll change that back to what it was.

pjfanning · 2023-06-22T20:04:02Z

If possible and the IDE makes it easy, I think keeping getEntry/hasEntry case-sensitive and switching most code to a set of case-sensitive methods makes releasing easier.

tballison · 2023-06-22T20:21:56Z

Should I add getEntryCaseInsensitive and hasEntryCaseInsensitive to the DirectoryEntry interface?

pjfanning · 2023-06-22T20:23:56Z

Should I add getEntryCaseInsensitive and hasEntryCaseInsensitive to the DirectoryEntry interface?

Seems best to expose them fully.

tballison · 2023-06-22T20:26:23Z

Agree, but won't that break any implementations of DirectoryEntry? I can't imagine anyone is actually doing this, but, it still would break backward compatibility, no?

pjfanning · 2023-06-22T20:31:55Z

I still think we should add it to the interface. The number of affected users will be low.

…nsitive; restore InternalWorkbook.WORKBOOK_DIR_ENTRY_NAMES

tballison · 2023-06-22T21:12:19Z

PR just got huge...LOL...

tballison · 2023-07-17T17:54:56Z

@pjfanning , wdyt? Ready to merge or further work required? Thanks, again!

pjfanning · 2023-07-17T18:15:21Z

Sure - seems ok to me. You'll need to rebase your branch before submitting to svn because a few changes have gone into trunk.

pjfanning · 2023-09-12T10:14:39Z

@tballison is this PR still something that you want to merge? It's a year since the last POI release and it would be good to get a release out in the next month or 2.

tballison · 2023-09-12T10:15:32Z

Y, will update it today and merge. Thank you!

tballison · 2023-09-15T20:30:17Z

Argh... Monday...

# Conflicts: # poi-scratchpad/src/main/java/org/apache/poi/hpbf/model/HPBFPart.java # poi-scratchpad/src/main/java/org/apache/poi/hslf/record/CurrentUserAtom.java # poi-scratchpad/src/main/java/org/apache/poi/hslf/usermodel/HSLFSlideShowImpl.java # poi-scratchpad/src/main/java/org/apache/poi/hwpf/HWPFDocumentCore.java # poi-scratchpad/src/test/java/org/apache/poi/hsmf/parsers/TestPOIFSChunkParser.java # src/resources/ooxml-lite-report.xsb

… failing tests.

tballison · 2023-09-18T15:22:03Z

I'm consistently getting two test failures now locally:

[TestAllFiles](file:///home/tallison/Intellij/my-poi/poi-integration/build/reports/tests/test/classes/org.apache.poi.stress.TestAllFiles.html). [Extracting - #443 document/clusterfuzz-testcase-minimized-POIXWPFFuzzer-6733884933668864.docx XWPF](file:///home/tallison/Intellij/my-poi/poi-integration/build/reports/tests/test/classes/org.apache.poi.stress.TestAllFiles.html#handleExtracting(String,%20FileHandlerKnown,%20String,%20Class,%20String)[443])
    [TestAllFiles](file:///home/tallison/Intellij/my-poi/poi-integration/build/reports/tests/test/classes/org.apache.poi.stress.TestAllFiles.html). [#443 document/clusterfuzz-testcase-minimized-POIXWPFFuzzer-6733884933668864.docx XWPF](file:///home/tallison/Intellij/my-poi/poi-integration/build/reports/tests/test/classes/org.apache.poi.stress.TestAllFiles.html#handleFile(String,%20FileHandlerKnown,%20String,%20Class,%20String)[443])

org.opentest4j.AssertionFailedError: document/clusterfuzz-testcase-minimized-POIXWPFFuzzer-6733884933668864.docx - failed for handler XWPFFileHandler:  ==> Unexpected exception thrown: org.apache.xmlbeans.XmlException: error: Element or attribute "a:" do not match QName production: QName::=(NCName:)?NCName.
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:152)
...
Caused by: org.apache.xmlbeans.XmlException: error: Element or attribute "a:" do not match QName production: QName::=(NCName:)?NCName.
...
Caused by: org.xml.sax.SAXParseException; systemId: file://; lineNumber: 2; columnNumber: 6188; Element or attribute "a:" do not match QName production: QName::=(NCName:)?NCName.
	at com.sun.org.apache.xerces.internal.util.ErrorHandlerWrapper.createSAXParseException(ErrorHandlerWrapper.java:204)
	at com.sun.org.apache.xerces.internal.util.ErrorHandlerWrapper.fatalError(ErrorHandlerWrapper.java:178)
	at com.sun.org.apache.xerces.internal.impl.XMLErrorReporter.reportError(XMLErrorReporter.java:399)
	at com.sun.org.apache.xerces.internal.impl.XMLErrorReporter.reportError(XMLErrorReporter.java:326)

centic9 · 2023-09-18T18:26:35Z

Sorry, my bad, different version of Java have different XML Parsers, should be fixed on latest trunk, please rebase/merge onto latest.

tballison · 2023-09-18T19:17:32Z

I _think_ I had already brought in your changes. I've since merged from upstream and am getting the same two exceptions.

…

On Mon, Sep 18, 2023 at 2:26 PM Dominik Stadler ***@***.***> wrote: Sorry, my bad, different version of Java have different XML Parsers, should be fixed on latest trunk, please rebase/merge onto latest. — Reply to this email directly, view it on GitHub <#477 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABTNNPXSZOGXHRGALNIQDLTX3CG6PANCNFSM6AAAAAAZO65RZM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

centic9 · 2023-09-18T20:44:52Z

As this file is now set to IGNORED in stress.xls it should not be executed at all, so if you see the same file-name "clusterfuzz-testcase-minimized-POIXWPFFuzzer-6733884933668864.docx" in the failures, the necessary change seems to be still missing.

CI-Jobs at https://ci-builds.apache.org/job/POI/ were failing as well and are now fixed, thus I am fairly confident that latest trunk has it fixed now.

tballison · 2023-09-20T19:16:12Z

Pushed merge remote-tracking. I'm getting a clean build now. Thank you @centic9 !

tballison · 2023-09-20T19:54:35Z

Looks like I don't have write access?! @pjfanning or @centic9 would you be able to merge? THANK YOU!

pjfanning · 2023-09-20T20:00:12Z

@tballison You need to commit this to svn. https://svn.apache.org/repos/asf/poi/trunk

The git repo is a read-only mirror afaik.

tballison · 2023-09-20T20:01:13Z

🤦

…

On Wed, Sep 20, 2023 at 4:00 PM PJ Fanning ***@***.***> wrote: @tballison <https://github.com/tballison> You need to commit this to svn. https://svn.apache.org/repos/asf/poi/trunk The git repo is a read-only mirror afaik. — Reply to this email directly, view it on GitHub <#477 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABTNNPWPIU2BR325IY7TBALX3NDNNANCNFSM6AAAAAAZO65RZM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

tballison · 2023-09-20T20:08:42Z

47950.zip
This patch looks about right. Will apply shortly. Sorry about that, and thank you, again, @pjfanning and @centic9!

pjfanning · 2023-10-15T11:16:18Z

@tballison could you close this? I believe that this was applied and was part of POI 5.2.4 release.

tballison · 2023-10-16T10:07:24Z

Committed: https://svn.apache.org/viewvc?view=revision&revision=1912438

tballison added 2 commits June 21, 2023 11:55

Bug47950 -- make stream/directory name lookup in OLE2 case insensitive

00e2296

47950 -- make OLE2 stream/directory look up case insensitive -- impro…

e7bc5a4

…ve documentation slightly

pjfanning reviewed Jun 21, 2023

View reviewed changes

poi/src/main/java/org/apache/poi/hssf/usermodel/HSSFWorkbook.java Outdated Show resolved Hide resolved

47950 -- fix typo, add hasCaseSensitiveEntry

ba2f875

47950 -- use hasCaseSensitiveEntry() where it belongs

37db4b8

47950 -- refactor to add hasEntryCaseInsensitive and getEntryCaseInse…

cffe531

…nsitive; restore InternalWorkbook.WORKBOOK_DIR_ENTRY_NAMES

tballison added 2 commits September 18, 2023 10:56

47950 -- fix bug added during conflict resolution -- still WIP -- two…

8b7d395

… failing tests.

Merge remote-tracking branch 'upstream/trunk' into 47950

970b160

Merge remote-tracking branch 'upstream/trunk' into 47950

02820e0

tballison closed this Oct 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug47950 -- make stream/directory name lookup in OLE2 case insensitive #477

Bug47950 -- make stream/directory name lookup in OLE2 case insensitive #477

tballison commented Jun 21, 2023

pjfanning Jun 21, 2023

tballison Jun 21, 2023

tballison commented Jun 21, 2023 •

edited

Loading

tballison commented Jun 22, 2023 •

edited

Loading

pjfanning commented Jun 22, 2023

tballison commented Jun 22, 2023 •

edited

Loading

pjfanning commented Jun 22, 2023

tballison commented Jun 22, 2023

pjfanning commented Jun 22, 2023

tballison commented Jun 22, 2023

pjfanning commented Jun 22, 2023

tballison commented Jun 22, 2023

tballison commented Jul 17, 2023

pjfanning commented Jul 17, 2023

pjfanning commented Sep 12, 2023

tballison commented Sep 12, 2023

tballison commented Sep 15, 2023

tballison commented Sep 18, 2023

centic9 commented Sep 18, 2023

tballison commented Sep 18, 2023 via email

centic9 commented Sep 18, 2023

tballison commented Sep 20, 2023

tballison commented Sep 20, 2023

pjfanning commented Sep 20, 2023

tballison commented Sep 20, 2023 via email

tballison commented Sep 20, 2023

pjfanning commented Oct 15, 2023

tballison commented Oct 16, 2023

Bug47950 -- make stream/directory name lookup in OLE2 case insensitive #477

Bug47950 -- make stream/directory name lookup in OLE2 case insensitive #477

Conversation

tballison commented Jun 21, 2023

pjfanning Jun 21, 2023

Choose a reason for hiding this comment

tballison Jun 21, 2023

Choose a reason for hiding this comment

tballison commented Jun 21, 2023 • edited Loading

tballison commented Jun 22, 2023 • edited Loading

pjfanning commented Jun 22, 2023

tballison commented Jun 22, 2023 • edited Loading

pjfanning commented Jun 22, 2023

tballison commented Jun 22, 2023

pjfanning commented Jun 22, 2023

tballison commented Jun 22, 2023

pjfanning commented Jun 22, 2023

tballison commented Jun 22, 2023

tballison commented Jul 17, 2023

pjfanning commented Jul 17, 2023

pjfanning commented Sep 12, 2023

tballison commented Sep 12, 2023

tballison commented Sep 15, 2023

tballison commented Sep 18, 2023

centic9 commented Sep 18, 2023

tballison commented Sep 18, 2023 via email

centic9 commented Sep 18, 2023

tballison commented Sep 20, 2023

tballison commented Sep 20, 2023

pjfanning commented Sep 20, 2023

tballison commented Sep 20, 2023 via email

tballison commented Sep 20, 2023

pjfanning commented Oct 15, 2023

tballison commented Oct 16, 2023

tballison commented Jun 21, 2023 •

edited

Loading

tballison commented Jun 22, 2023 •

edited

Loading

tballison commented Jun 22, 2023 •

edited

Loading