Uncaught IllegalArgumentException (Skeleton and ogg page)

[floyd-fuh]

During a fuzzing run with the AFL-based Kelinci fuzzer found at https://github.com/isstac/kelinci I found the two attached files result in IllegalArgumentException:

Exception in thread "main" java.lang.IllegalArgumentException: Unsupported Skeleton message offset 12336 detected
    at org.gagravarr.skeleton.SkeletonFisbone.<init>(SkeletonFisbone.java:65)
    at org.gagravarr.skeleton.SkeletonPacketFactory.create(SkeletonPacketFactory.java:64)
    at org.gagravarr.tika.OggDetector.detect(OggDetector.java:126)

Exception in thread "main" java.lang.IllegalArgumentException: Found Ogg page in format 16 but we only support version 0
    at org.gagravarr.ogg.OggPage.<init>(OggPage.java:51)
    at org.gagravarr.ogg.OggPacketReader.getNextPacket(OggPacketReader.java:118)
    at org.gagravarr.tika.OggDetector.detect(OggDetector.java:99)

Shouldn't the library throw a more generic error message such as a parser error?

IllegalArgumentException.zip

[Gagravarr]

Based on https://wiki.apache.org/tika/ErrorsAndExceptions I'm tempted to say that the detailed exception should remain at the org.gagravarr.ogg level, but should be caught and re-thrown as a TikaException in the org.gagravarr.tika code.

Would that be what you, as an end-user, expect?

[floyd-fuh]

That would definitely satisfy my needs as an end-user. I'm definitely not the software design pattern engineering guy who could answer this question properly, but I try anyway: I think every library should probably declare the exceptions it could throw. I mean in the end it is this library's responsibility to communicate to tika which exceptions could be thrown. From that point of view I would say an entire file is indeed an argument to the ogg code, but usually an IllegalArgumentException is something I would expect when I specified a wrong argument. However, in this case the ogg code could read the file, but was just not happy with it's content. As stated in some discussions about this kind of things (e.g. https://stackoverflow.com/questions/15208544/when-should-an-illegalargumentexception-be-thrown ), IllegalArgumentException is usually something thrown when the programmer did something wrong (not the case here). So from that point of view I think throwing something else than IllegalArgumentException would be better.

Another point I see is that catching a runtime exception as early as possible and throw some sane and excpected Exception that was declared to be thrown is a good idea.

But please don't take my word, I'm not a programmer on a daily basis and I don't think about these things often enough :)

[Gagravarr]

In 3ea1afb02bc2029b05283b10314354da9bbe1eaa I've changed the low-level code to throw an UnsupportedOperationException instead. In d6302508ffa634a5b6130e89bed11b857080c73a I then updated the Detector to catch these and say "not one of mine" rather than propogating the exception. (The Tika detect method signature doesn't provide a way to pass a message up for this kind of case)

Does that look + work ok for you?

[floyd-fuh]

That sounds like a good solution! I checked the code and from my point of view that makes sense and will work for me.

[Gagravarr]

Great, thanks for the help!