java.lang.StringIndexOutOfBoundsException processing a .JPG file

[mbmast]

Getting StringIndexOutOfBoundsException on the included .JPG file. Here's the code:

metadata = ImageMetadataReader.readMetadata( pathSourceFile.toFile() );

// ImageMetadataReader returns dictionaries that we can easily drill
// into to produce the key value pairs DirectoryNameBuilder uses to
// construct the target directory name.
for (Directory directory : metadata.getDirectories()) {
	for (Tag tag : directory.getTags()) {
		String key = directory.getName() + "|" + tag.getTagName();
		String value = tag.getDescription();
		
		// I've never seen a null key, but Panasonic cameras build dictionaries with plenty of null values. 
		if (key == null || value == null) {
			continue;
		}
		
		sortedMap.put(key, value);
	}
}

Exception in thread "main" java.lang.StringIndexOutOfBoundsException: Range [12, 12 + 65534) out of bounds for length 108
	at java.base/jdk.internal.util.Preconditions$1.apply(Preconditions.java:55)
	at java.base/jdk.internal.util.Preconditions$1.apply(Preconditions.java:52)
	at java.base/jdk.internal.util.Preconditions$4.apply(Preconditions.java:213)
	at java.base/jdk.internal.util.Preconditions$4.apply(Preconditions.java:210)
	at java.base/jdk.internal.util.Preconditions.outOfBounds(Preconditions.java:98)
	at java.base/jdk.internal.util.Preconditions.outOfBoundsCheckFromIndexSize(Preconditions.java:118)
	at java.base/jdk.internal.util.Preconditions.checkFromIndexSize(Preconditions.java:397)
	at java.base/java.lang.String.checkBoundsOffCount(String.java:4951)
	at java.base/java.lang.String.<init>(String.java:1523)
	at com.drew.metadata.icc.IccDescriptor.getTagDataString(IccDescriptor.java:94)
	at com.drew.metadata.icc.IccDescriptor.getDescription(IccDescriptor.java:63)
	at com.drew.metadata.Directory.getDescription(Directory.java:1147)
	at com.drew.metadata.Tag.getDescription(Tag.java:76)
	at com.mbm.mediaorganizer.DirectoryNameBuilder.getTags(DirectoryNameBuilder.java:252)
	at com.mbm.mediaorganizer.DirectoryNameBuilder.getDirectoryName(DirectoryNameBuilder.java:68)
	at com.mbm.mediaorganizer.RelocateFileVisitor.relocateFile(RelocateFileVisitor.java:240)
	at com.mbm.mediaorganizer.RelocateFileVisitor.visitFile(RelocateFileVisitor.java:96)
	at com.mbm.mediaorganizer.RelocateFileVisitor.visitFile(RelocateFileVisitor.java:1)
	at java.base/java.nio.file.Files.walkFileTree(Files.java:2810)
	at java.base/java.nio.file.Files.walkFileTree(Files.java:2881)
	at com.mbm.mediaorganizer.MediaOrganizer.run(MediaOrganizer.java:102)
	at com.mbm.mediaorganizer.MediaOrganizer.main(MediaOrganizer.java:42)

[mbmast]

Forgot to mention.... I'm using metadata-extractor-2.19.0.jar.

[drewnoakes]

Thanks for the bug report and sample image. Will happily accept a pull request for this.

[DAN-MU-ZI]

Upon reviewing the attached image, I noticed that there are multiple desc tags within the ICC profile. From my understanding, the desc tag should typically appear only once. However, in this case, it seems to be recorded multiple times.

Using HxD to analyze the file, I observed that the first desc tag appears to be corrupted. Additionally, it seems plausible that the desc data was split into multiple entries due to its excessive length.

I would like to ask for guidance on how to handle this kind of situation. Is there a proper way to interpret these split entries in ICC tags? Moreover, are there any recommendations for recovering corrupted data in such cases?

Lastly, I kindly ask for your understanding as this message was translated from Korean to English, and there might be some unnatural phrasing.

[drewnoakes]

안녕하세요! 의견 주셔서 감사합니다. 저는 사실 한국어를 공부하고 있는 학생이에요. 아내가 한국 사람이라서 집에서 조금씩 한국어로 대화하고 있어요.

It seems like the exception is coming from here:

metadata-extractor/Source/com/drew/metadata/icc/IccDescriptor.java

Lines 92 to 94 in 0c3452b

	case ICC_TAG_TYPE_DESC:
	int stringLength = reader.getInt32(8);
	return new String(bytes, 12, stringLength - 1);

In this case we read stringLength, then try to use it. However, we don't validate stringLength against the length of bytes, which seems like a bad idea.

I think we should add some validation, and throw a BufferBoundsException when we observe a problem. That exception will be caught by the method, and a null description will be returned instead.

It may be that other tags after this are corrupted too, and that the library produces some junk data.

[DAN-MU-ZI]

Hello, @drewnoakes! First of all, it's great to hear that you are learning Korean.

I reviewed the code and attempted to validate the stringLength during processing. If the data is invalid, an exception is thrown. The code is as follows:

case ICC_TAG_TYPE_DESC:
    int stringLength = reader.getInt32(8);

    if (stringLength < 1 || stringLength > (bytes.length - 12))
        throw new BufferBoundsException(
            String.format("Invalid stringLength in ICC profile. Found: %d, Expected: between 1 and %d.",
                          stringLength, bytes.length - 12)
        );

    return new String(bytes, 12, stringLength - 1);

The data validation works correctly, but I realized the need to devise a better method instead of relying on the tag call. Additionally, I confirmed that other tags are functioning correctly.

I will think about this further and share an improved solution once I have it.

Thank you!

[drewnoakes]

Looks good! I think a zero length should be valid. Also the reception constructor has an overload that takes numbers and constructs the message for you.

[DAN-MU-ZI]

Hello @drewnoakes,
Thank you for your feedback earlier. Based on your suggestions, I've made a few updates.

1. Validation of stringLength
   As per your suggestion, I've ensured that a stringLength value of zero is treated as valid.
   Now, BufferBoundsException is only thrown if stringLength is negative or exceeds bytes.length - 12.
   With this chage, a stringLength of zero will result in an empty string being returned.

2. Utilizing Overloaded Exception Constructor
   I've referenced the overloaded constructor of BufferBoundsException to simplify the way messages are generated.
   Here is the updated code:

case ICC_TAG_TYPE_DESC:
    int stringLength = reader.getInt32(8);

    if (stringLength < 0 || stringLength > (bytes.length - 12)) {
        throw new BufferBoundsException(12, stringLength, bytes.length);
    }

    return new String(bytes, 12, stringLength - 1);

Please let me know if further review or improvements are needed.
Thank you again for your clear and helpful feedback!

[drewnoakes]

That looks great to me @DAN-MU-ZI. Would you like to submit the change as a pull request?