search

dragon66 / icafe

Java library for reading, writing, converting and manipulating images and metadata
Eclipse Public License 1.0
204 stars 58 forks source link

IndexOutOfBoundsException risk in TIFFTweaker.writeMultipageTIFF() #63

Closed THausherr closed 6 years ago

THausherr commented 6 years ago

The first loop (at line 3344) in TIFFTweaker.writeMultipageTIFF() may throw an exception which results in a stack trace being written. If the exception happens before list.add(writer.getIFD());, then list could be empty or too small at line 3358 in the second loop., which would result in an IndexOutOfBoundsException.

Same in the other method with the same name.

=> consider rethrowing the exception.

PS: ignore my mail from this morning. I was in a hurry.

dragon66 commented 6 years ago

@THausherr Thanks for pointing this out. Instead of throwing new exception, I would rather use the list size instead of the image array size to perform the second loop and in the end check to make sure at least we have one page in the list before writing it to the output. I have checked in the changes.

THausherr commented 6 years ago

But doesn't this bring the risk that if there's a (swallowed) exception, that not all pages are written and the user won't notice it?

dragon66 commented 6 years ago

@THausherr That's a tradeoff. Depending on the users' expectation. Sometimes it's better than nothing. If you have a concern, log something when the pages expected are not the same as the pages actually created. You still want to fix the issue when not all the PDF pages are rendered as BufferedImages anyway.

dragon66 commented 6 years ago

After much struggling, I have finally decided to replace e.printStackTrace() with logging and throwing RuntimeException.

THausherr commented 6 years ago

Consider changing throw new RuntimeException("Failed writing page " + pageNumber); to throw new RuntimeException("Failed writing page " + pageNumber, e); for people who have turned logging off, or don't have access to it.

RuntimeException is a bit of an overkill... might abort the calling application. IMHO an IOException would be better.

dragon66 commented 6 years ago

@THausherr good point. Checked exception will be favored where at least partial result is acceptable like in multipage TIFF creation case.

I have made some change to replace RuntimeException with customized exception where applicable.