[Bug 61337] New: Downgrade AssertionError to RuntimeException or something better

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 61337] New: Downgrade AssertionError to RuntimeException or something better

Bugzilla from bugzilla@apache.org
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: [hidden email]
          Reporter: [hidden email]
  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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 61337] Downgrade AssertionError to RuntimeException or something better

Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61337

Tim Allison <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 OS|                            |All

--- Comment #1 from Tim Allison <[hidden email]> ---
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 61337] Downgrade AssertionError to RecordFormatException?

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61337

Tim Allison <[hidden email]> 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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 61337] Downgrade AssertionError to RecordFormatException?

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61337

--- Comment #2 from Tim Allison <[hidden email]> ---
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 61337] Downgrade AssertionError to RecordFormatException?

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61337

--- Comment #3 from Andreas Beeker <[hidden email]> ---
+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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 61337] Downgrade AssertionError to RecordFormatException?

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61337

--- Comment #4 from Tim Allison <[hidden email]> ---
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 61337] Downgrade AssertionError to RecordFormatException?

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61337

--- Comment #5 from Tim Allison <[hidden email]> ---
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: [hidden email]
For additional commands, e-mail: [hidden email]

Loading...