New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Uncaught IllegalArgumentException (Skeleton and ogg page) #27

Closed
floyd-fuh opened this Issue Apr 17, 2018 · 5 comments

Comments

Projects
None yet
2 participants
@floyd-fuh

floyd-fuh commented Apr 17, 2018

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

This comment has been minimized.

Show comment
Hide comment
@Gagravarr

Gagravarr Apr 18, 2018

Owner

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?

Owner

Gagravarr commented Apr 18, 2018

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

This comment has been minimized.

Show comment
Hide comment
@floyd-fuh

floyd-fuh Apr 19, 2018

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 :)

floyd-fuh commented Apr 19, 2018

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 added a commit that referenced this issue Apr 20, 2018

Return a slightly more helpful exception (UnsupportedOperationExcepti…
…on) for formats we cannot handle (often corrupt files) #27

Gagravarr added a commit that referenced this issue Apr 20, 2018

#27 For invalid Ogg Pages or broken Skeleton streams, have the Tika O…
…ggDetector just decline to detect, rather than raising a runtime exception
@Gagravarr

This comment has been minimized.

Show comment
Hide comment
@Gagravarr

Gagravarr Apr 20, 2018

Owner

In 3ea1afb I've changed the low-level code to throw an UnsupportedOperationException instead. In d630250 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?

Owner

Gagravarr commented Apr 20, 2018

In 3ea1afb I've changed the low-level code to throw an UnsupportedOperationException instead. In d630250 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

This comment has been minimized.

Show comment
Hide comment
@floyd-fuh

floyd-fuh Apr 20, 2018

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

floyd-fuh commented Apr 20, 2018

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

andrm added a commit to andrm/VorbisJava that referenced this issue Apr 20, 2018

Return a slightly more helpful exception (UnsupportedOperationExcepti…
…on) for formats we cannot handle (often corrupt files) Gagravarr#27

andrm added a commit to andrm/VorbisJava that referenced this issue Apr 20, 2018

Gagravarr#27 For invalid Ogg Pages or broken Skeleton streams, have t…
…he Tika OggDetector just decline to detect, rather than raising a runtime exception
@Gagravarr

This comment has been minimized.

Show comment
Hide comment
@Gagravarr

Gagravarr Apr 20, 2018

Owner

Great, thanks for the help!

Owner

Gagravarr commented Apr 20, 2018

Great, thanks for the help!

@Gagravarr Gagravarr closed this Apr 20, 2018

croger42 added a commit to croger42/VorbisJava that referenced this issue Apr 21, 2018

Return a slightly more helpful exception (UnsupportedOperationExcepti…
…on) for formats we cannot handle (often corrupt files) Gagravarr#27

croger42 added a commit to croger42/VorbisJava that referenced this issue Apr 21, 2018

Gagravarr#27 For invalid Ogg Pages or broken Skeleton streams, have t…
…he Tika OggDetector just decline to detect, rather than raising a runtime exception

croger42 added a commit to croger42/VorbisJava that referenced this issue Apr 21, 2018

Return a slightly more helpful exception (UnsupportedOperationExcepti…
…on) for formats we cannot handle (often corrupt files) Gagravarr#27

croger42 added a commit to croger42/VorbisJava that referenced this issue Apr 21, 2018

Gagravarr#27 For invalid Ogg Pages or broken Skeleton streams, have t…
…he Tika OggDetector just decline to detect, rather than raising a runtime exception

croger42 added a commit to croger42/VorbisJava that referenced this issue Aug 17, 2018

Return a slightly more helpful exception (UnsupportedOperationExcepti…
…on) for formats we cannot handle (often corrupt files) Gagravarr#27

croger42 added a commit to croger42/VorbisJava that referenced this issue Aug 17, 2018

Gagravarr#27 For invalid Ogg Pages or broken Skeleton streams, have t…
…he Tika OggDetector just decline to detect, rather than raising a runtime exception
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment