Potential OOME reading text file as image

[tmssngr]

Our SWT-based application tries to read files first as image before it tries to read it as a text. This works quite good in detecting images. However, a customer found now a text file which causes an OOME because of missing sanity checks in the image loader code.

Describe the bug
new ImageData(stream) interpretes even extremely large image width/height as valid and tries to load them; potentially resulting in an OOME.

To Reproduce
Use this snippet

import java.io.*;
import java.nio.file.*;

import org.eclipse.swt.*;
import org.eclipse.swt.graphics.*;
import org.eclipse.swt.layout.*;
import org.eclipse.swt.widgets.*;

final class ImageDisplayer {

	public static void main(String[] args) throws IOException {
		if (args.length != 1) {
			return;
		}

		final ImageData imageData;
		try (InputStream stream = Files.newInputStream(Paths.get(args[0]))) {
			imageData = new ImageData(stream);
		}

		final Display display = new Display();
		final Image image = new Image(display, imageData);

		final Shell shell = new Shell(display);
		shell.setLayout(new FillLayout());

		final Label label = new Label(shell, SWT.NONE);
		label.setImage(image);

		shell.setSize(400, 300);
		shell.open();

		while (!shell.isDisposed()) {
			if (!display.readAndDispatch()) {
				display.sleep();
			}
		}

		image.dispose();
		display.dispose();
	}
}

Run with -Xmx256m and specify a file with this content

BM2D_003.fxf &
    21222-2259+22_EF1.55_KTT1_INR_LNR_2esbnf_tjg ^

Expected behavior
I expect to get an SWT-error for invalid image, but I'm getting an OOME.

Environment:

1. Select the platform(s) on which the behavior is seen:

• • All OS
• • Windows
• • Linux
• • macOS

[laeubi]

However, a customer found now a text file which causes an OOME because of missing sanity checks in the image loader code.

Might one argue that the sanity check is missing in the client code that tries to load large text files as images? ;-)

Our SWT-based application tries to read files first as image before it tries to read it as a text. This works quite good in detecting images.

Are you really depend on the native image formats (e.g. GTK?) to be supported? If not it seems much cleaner to check for the magic bytes in detecting images over text. Also a common technique is to check for \0 in the file (what makes it likely not a text file).

new ImageData(stream) interpretes even extremely large image width/height as valid and tries to load them

I must confess I don't understand how one would validate this. If width/height is negative I would agree but why should it not be allowed to load extremely large image with SWT?

[tmssngr]

Might one argue that the sanity check is missing in the client code that tries to load large text files as images? ;-)

How to achieve that exactly? Shouldn't SWT already contain these checks to verify whether it can load this or that format?

I must confess I don't understand how one would validate this. If width/height is negative I would agree but why should it not be allowed to load extremely large image with SWT?

Should SWT really load images with sizes larger than, let's say 100,000 pixels in each dimension?

[HeikoKlare]

I am not yet sure about the exact expected behavior here. I agree that an OOME is nothing one expects with an actually valid input (such as a large file), but I am also not sure what should happen instead (from SWT side).

Should SWT really load images with sizes larger than, let's say 100,000 pixels in each dimension?

To me the question is where a limit should be drawn. There is nothing in the API stating that only images up to a specific are supported. And I guess whether an image at a specific size can be loaded or not will also depend on the heap size, won't it? If someone wants to load quite large images, they may increase the heap size of their application accordingly and/or add according checks for the supported sizes of images to be loaded, as also indicated by Christoph.

I have to admit that I did not yet see any reports regarding OOMEs (both in the Eclipse repos as well as for our RCP product) because of trying to load images that are too large. Even other applications often crash if you try to load something that is too large. E.g., I use to run into that when opening two 1GB+ text files in VS code.

[laeubi]

How to achieve that exactly?

Usually one creates a buffered stream, mark the current position with a large enough limit (e.g. 10 bytes), then read the first bytes and check common byte marks see for example:

https://en.wikipedia.org/wiki/List_of_file_signatures

And as said, usually if one want to detect binary versus text files searching for Zero bytes usually gives a quite good way to distinguish that as these should never be part of a text file (be careful with UTF BOM marks!) but usually appear in binary files.

Shouldn't SWT already contain these checks to verify whether it can load this or that format?

Maybe one want to enhance SWT here yes. But of course it also depends on what exact thing is used to read an image. And if you look at the list you will see that Bitmap files start with 42 4D ( = BM ) so one would then maybe add more validations, e.g read additional headers and so on. Maybe that's not so easy on an input stream though.

I have to admit that I did not yet see any reports regarding OOMEs (both in the Eclipse repos as well as for our RCP product) because of trying to load images that are too large.

It all depends on the file format and the color depth and the maximum memory of the JVM of course, for "usual" images it is just a few megabytes.

[tmssngr]

How to achieve that exactly?

Usually one creates a buffered stream, mark the current position with a large enough limit (e.g. 10 bytes), then read the first bytes and check common byte marks see for example:

https://en.wikipedia.org/wiki/List_of_file_signatures

That's what SWT does internally and it detects that it potentially could load this file. So you suggest that our code should check that for all SWT supported image file formats, too, but better?

And as said, usually if one want to detect binary versus text files searching for Zero bytes usually gives a quite good way to distinguish that as these should never be part of a text file (be careful with UTF BOM marks!) but usually appear in binary files.

If I would search for \0 files, I would exclude all text-file based images, e.g. SVG.

I've checked with Eclipse and it avoids this problem by simply delegating the display of images to the Browser.

[laeubi]

That's what SWT does internally and it detects that it potentially could load this file.

The problem is that SWT usually uses a stream of bytes, you might can improve it by implement more of the BMP specification and check if the file-syste could match the expected numbers of pixels.

If I would search for \0 files, I would exclude all text-file based images, e.g. SVG.

It is not really hard to search for <svg in the first line of a textfile....

I've checked with Eclipse and it avoids this problem by simply delegating the display of images to the Browser.

There are may ways yes what works best depends on the use-case. But if you ask SWT to load an image it will assume you want an image and are quite certain to get one. It is not an API to detect images in general.

[tmssngr]

So you suggest that our code should check that for all SWT supported image file formats, too, but better?