You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@poi.apache.org by bu...@apache.org on 2017/07/25 11:12:54 UTC
[Bug 61337] New: Downgrade AssertionError to RuntimeException or
something better
https://bz.apache.org/bugzilla/show_bug.cgi?id=61337
Bug ID: 61337
Summary: Downgrade AssertionError to RuntimeException or
something better
Product: POI
Version: 3.17-dev
Hardware: PC
Status: NEW
Severity: normal
Priority: P2
Component: HWPF
Assignee: dev@poi.apache.org
Reporter: tallison@mitre.org
Target Milestone: ---
Created attachment 35171
--> https://bz.apache.org/bugzilla/attachment.cgi?id=35171&action=edit
triggering file
A fuzzed version of testException2.doc in Tika's test corpus triggers:
Caused by: java.lang.AssertionError
at org.apache.poi.hwpf.usermodel.Range.sanityCheck(Range.java:1158)
at org.apache.poi.hwpf.usermodel.Range.<init>(Range.java:195)
at
org.apache.poi.hwpf.usermodel.HeaderStories.getSubrangeAt(HeaderStories.java:357)
at
org.apache.poi.hwpf.usermodel.HeaderStories.getOddHeaderSubrange(HeaderStories.java:196)
at
org.apache.tika.parser.microsoft.WordExtractor.parse(WordExtractor.java:180)
at
org.apache.tika.parser.microsoft.OfficeParser.parse(OfficeParser.java:175)
at
org.apache.tika.parser.microsoft.OfficeParser.parse(OfficeParser.java:131)
at
org.apache.tika.parser.CompositeParser.parse(CompositeParser.java:280)
at
org.apache.tika.parser.CompositeParser.parse(CompositeParser.java:280)
at
org.apache.tika.parser.AutoDetectParser.parse(AutoDetectParser.java:135)
We should definitely have a range check, but I think it would be better to
throw a RecordFormatException or RuntimeException?
More generally, there is room to make our "broken record" exception handling
more consistent. My preference would be to throw RecordFormatException for
this kind of thing.
Other ideas?
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 61337] Downgrade AssertionError to RecordFormatException?
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=61337
Tim Allison <ta...@mitre.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Summary|Downgrade AssertionError to |Downgrade AssertionError to
|RuntimeException or |RecordFormatException?
|something better |
Component|HWPF |POI Overall
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 61337] Downgrade AssertionError to RecordFormatException?
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=61337
--- Comment #4 from Tim Allison <ta...@mitre.org> ---
I'll probably start on this one class/family at a time unless there are
objections.
It looks like there are basically 4 places where we rely on assert.
1) assert that things are or aren't null, as in DrawTextParagraph:
String buFontStr = bulletStyle.getBulletFont();
if (buFontStr == null) {
buFontStr = paragraph.getDefaultFontFamily();
}
assert(buFontStr != null);
FontInfo buFont = new DrawFontInfo(buFontStr);
2) assert instanceof, as in Range:
assert ( _doc instanceof HWPFDocument );
3) assert x == y to confirm that a record is not wonky, as in HwmfBitmapDib:
assert(introSize == headerSize);
or in LittleEndianByteArrayInputStream:
assert skipped == size : "Buffer overrun";
4) checks on limitations of implementation as in:
assert false : "hashCode not designed";
There may be other uses as well...
Proposed solutions:
1) RecordFormatException? Or something else?
2) What exception should we use here?
3) is straightforward, I think: RecordFormatException
4) is straightforward, I think: implement/auto-generate a hashcode
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 61337] Downgrade AssertionError to RecordFormatException?
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=61337
--- Comment #3 from Andreas Beeker <ki...@apache.org> ---
+1 for replacing java-assert
I think we need at least two different cases:
asserts based on record format (throws RecordFormatException)
and other asserts (throws ? extends RuntimeException)
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 61337] Downgrade AssertionError to RecordFormatException?
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=61337
--- Comment #2 from Tim Allison <ta...@mitre.org> ---
Of course, there are many, many more without the paren: "assert "
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 61337] Downgrade AssertionError to RuntimeException or
something better
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=61337
Tim Allison <ta...@mitre.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
OS| |All
--- Comment #1 from Tim Allison <ta...@mitre.org> ---
I see 73 instances of "assert(", and, yes, several of these are my fault.
In most cases, I'd think we could convert these to a RecordFormatException. If
there were a use case for turning assertions off and hoping for the best, I'd
want to leave the asserts in. However, it looks (on quick review) like turning
the assertions off will yield corrupt objects/data. So, I don't see a use case
for assert instead of a RecordFormatException.
I'm happy to make the changes, but given that this will be not a small patch,
I'd like to get feedback before I fix this globally.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 61337] Downgrade AssertionError to RecordFormatException?
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=61337
--- Comment #5 from Tim Allison <ta...@mitre.org> ---
in r1803092, I made some modifications to hwpf.Range. I left in the asserts in
the binary search code.
I added DocumentFormatException as a RuntimeException to handle larger scale
problems with parsing the document than RecordFormatException. I think I'd
want to use this for 1) and 2), RecordFormatException for 3), and just
implement 4).
I'll wait a bit before making any other changes to give folks a chance to
review and offer feedback.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org