Multi-byte filenames in zip files can cause an endless loop in ZipString.hash

[jhackel-hypo]

Our Spring Boot 3.2.0 app hangs for random builds during a very early start-up phase. We see this thread-dump:

"main" #1 prio=5 os_prio=31 cpu=17588,58ms elapsed=17,96s tid=0x0000000145009a00 nid=0x2203 runnable  [0x000000016d49a000]
   java.lang.Thread.State: RUNNABLE
	at org.springframework.boot.loader.zip.ZipString.readInBuffer(ZipString.java:281)
	at org.springframework.boot.loader.zip.ZipString.hash(ZipString.java:112)
	at org.springframework.boot.loader.zip.ZipContent$Loader.add(ZipContent.java:445)
	at org.springframework.boot.loader.zip.ZipContent$Loader.loadContent(ZipContent.java:581)
	at org.springframework.boot.loader.zip.ZipContent$Loader.openAndLoad(ZipContent.java:543)
	at org.springframework.boot.loader.zip.ZipContent$Loader.loadNestedZip(ZipContent.java:537)
	at org.springframework.boot.loader.zip.ZipContent$Loader.load(ZipContent.java:522)
	at org.springframework.boot.loader.zip.ZipContent.open(ZipContent.java:372)
	at org.springframework.boot.loader.zip.ZipContent.open(ZipContent.java:361)
	at org.springframework.boot.loader.jar.NestedJarFileResources.<init>(NestedJarFileResources.java:57)
	at org.springframework.boot.loader.jar.NestedJarFile.<init>(NestedJarFile.java:141)
	at org.springframework.boot.loader.jar.NestedJarFile.<init>(NestedJarFile.java:120)
	at org.springframework.boot.loader.net.protocol.jar.UrlNestedJarFile.<init>(UrlNestedJarFile.java:42)
	at org.springframework.boot.loader.net.protocol.jar.UrlJarFileFactory.createJarFileForNested(UrlJarFileFactory.java:86)
	at org.springframework.boot.loader.net.protocol.jar.UrlJarFileFactory.createJarFile(UrlJarFileFactory.java:55)
	at org.springframework.boot.loader.net.protocol.jar.UrlJarFiles.getOrCreate(UrlJarFiles.java:72)
	at org.springframework.boot.loader.net.protocol.jar.JarUrlConnection.connect(JarUrlConnection.java:289)
	at org.springframework.boot.loader.net.protocol.jar.JarUrlConnection.getJarFile(JarUrlConnection.java:99)
	at org.springframework.boot.loader.net.protocol.jar.JarUrlClassLoader.getJarFile(JarUrlClassLoader.java:185)
	at org.springframework.boot.loader.net.protocol.jar.JarUrlClassLoader.definePackage(JarUrlClassLoader.java:143)
	at org.springframework.boot.loader.net.protocol.jar.JarUrlClassLoader.definePackageIfNecessary(JarUrlClassLoader.java:126)
	at org.springframework.boot.loader.net.protocol.jar.JarUrlClassLoader.loadClass(JarUrlClassLoader.java:99)
	at org.springframework.boot.loader.launch.LaunchedClassLoader.loadClass(LaunchedClassLoader.java:91)
	at java.lang.ClassLoader.loadClass(java.base@17.0.5/ClassLoader.java:520)
	at java.lang.Class.getDeclaredMethods0(java.base@17.0.5/Native Method)
	at java.lang.Class.privateGetDeclaredMethods(java.base@17.0.5/Class.java:3402)
	at java.lang.Class.getDeclaredMethod(java.base@17.0.5/Class.java:2673)
	at org.springframework.boot.loader.launch.Launcher.launch(Launcher.java:89)
	at org.springframework.boot.loader.launch.Launcher.launch(Launcher.java:53)
	at org.springframework.boot.loader.launch.JarLauncher.main(JarLauncher.java:58)

I am not sure what this code exactly does, but it seems to calculate a hash code of a a file name from a JAR file embedded in the super-jar of our app. The code seems to operate on DataBlock instances of the central ZIP file directory.

The error occurs because the block it is processing happens to only contain the first half of the file name. Unfortunately, the last byte in the data block is the first byte of multi-byte character. It seems, this code tries to read the whole multi-byte character:

spring-boot/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/zip/ZipString.java

Lines 117 to 121 in a242bd8

	if (!hasEnoughBytes(byteIndex, codePointSize, count)) {
	pos--;
	len++;
	break;
	}

But the data block seems to be unable to provide the missing byte, because it will not read any further data from the underlying file channel. It returns only the first byte of the multi-byte character again and again. And the hash function hangs.

I am not deep enough into it, do dig what is exactly going on. Furthermore, I am unable to provide the broken JAR as it comes from a closed source project.

[wilkinsona]

Thanks for the report.

I am unable to provide the broken JAR as it comes from a closed source project

Any jar that reproduces the problem would be useful so that we can reproduce the problem, or even just a concrete example of the file names that cause the problem.

[jhackel-hypo]

The ZIP record for the file as produced by zipdump is this:

000c8687: PK.0304: 0014 0800 0008 5788834c 04267862 00000fad 00002e4f 0054 0000 | 000c86a5 000c86f9 000c86f9 000c96a6 - de/genopace/bsh/pe/darlehen/validierung/FehlendeOderUngültigeBaufiSmartKosten.class

The path has the length 0x54 == 84. The data block happens to contain only 56 bytes of that path, which is the first half of the character ü.

[philwebb]

I wonder if we've already fixed this with #38572. @jhackel-hypo Any chance you could try 3.2.1-SNAPSHOT before we try anything else?

[jhackel-hypo]

I have no way to deliberately build a broken JAR. It just happens from time to time.

On the main branch, this test will also hang ZipStringTest:

@Test
void hashDataBlockWithInCompleteMultiByteCharacterAtTheEnd() throws IOException {
	ByteArrayDataBlock dataBlock = new ByteArrayDataBlock(
			(byte) 100, (byte) 101, (byte) 47, (byte) 103, (byte) 101, (byte) 110,
			(byte) 111, (byte) 112, (byte) 97, (byte) 99, (byte) 101,  (byte) 47,
			(byte) 98, (byte) 115, (byte) 104, (byte) 47, (byte) 112, (byte) 101,
			(byte) 47, (byte) 100, (byte) 97, (byte) 114, (byte) 108, (byte) 101,
			(byte) 104, (byte) 101, (byte) 110, (byte) 47, (byte) 118, (byte) 97,
			(byte) 108, (byte) 105, (byte) 100, (byte) 105, (byte) 101, (byte) 114,
			(byte) 117, (byte) 110, (byte) 103, (byte) 47, (byte) 70, (byte) 101,
			(byte) 104, (byte) 108,	(byte) 101, (byte) 110, (byte) 100, (byte) 101,
			(byte) 79, (byte) 100, (byte) 101, (byte) 114, (byte) 85, (byte) 110,
			(byte) 103,	(byte) -61
	);

	ZipString.hash(null, dataBlock, 0, 87, true);
}

This pretty much the same which ZipContent.Loader.add feeds to the function in my error case, except that there is more data at the beginning of the data block.

I do not really grasp how ZipContent.Loader.add ever deals with file names reaching beyond the end of the data block.

[jhackel-hypo]

It seems to me that ZipContent.Loader.add must never feed a split string to ZipString.hash. In fact, that should be a precondition when calling ZipString.hash with a data block.

[jhackel-hypo]

I think, I do understand the problem now. I am also pretty sure that 8c7e877 is not fixing the problem.

The problem is, that ZipString#hash is assuming, it can read the whole multibyte character by stepping a byte backwards (line 118) and reading from the FileChannelDataBlock again (line 112):

spring-boot/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/zip/ZipString.java

Lines 111 to 121 in a242bd8

	while (len > 0) {
	int count = readInBuffer(dataBlock, pos, buffer, len);
	len -= count;
	pos += count;
	for (int byteIndex = 0; byteIndex < count;) {
	int codePointSize = getCodePointSize(bytes, byteIndex);
	if (!hasEnoughBytes(byteIndex, codePointSize, count)) {
	pos--;
	len++;
	break;
	}

But this is not the case. The data block has half of the multibyte character at the last position of its FileChannelDataBlock.ManagedFileChannel#buffer. Stepping one position backward will return this first half of the multibyte character but not the second byte because it is not in the buffer. So, the FileChannelDataBlock#read method will return 1, because a single byte could be read. It will only read more bytes into its buffer, if the caller is asking for a position beyond what is already buffered (line 181):

spring-boot/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/zip/FileChannelDataBlock.java

Lines 179 to 183 in c50172d

	int read(ByteBuffer dst, long position) throws IOException {
	synchronized (this.lock) {
	if (position < this.bufferPosition || position >= this.bufferPosition + this.bufferSize) {
	fillBuffer(position);
	}

But because ZipString.hash is always stepping back one position, it will never go beyond the limits of the buffer. It will receive the same byte again and again.

[jhackel-hypo]

The implementation of ZipString#hash is making the wrong assumption that it can always read two (actually up to four) consecutive bytes from the data block. This is not the case. The clients of DataBlock#read must be able to process the data byte by byte.

[jhackel-hypo]

I guess a possible solution could be to use DataBlock#readyFully here:

spring-boot/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/zip/ZipString.java

Lines 276 to 286 in a242bd8

	private static int readInBuffer(DataBlock dataBlock, long pos, ByteBuffer buffer, int maxLen) throws IOException {
	buffer.clear();
	if (buffer.remaining() > maxLen) {
	buffer.limit(maxLen);
	}
	int count = dataBlock.read(buffer, pos);
	if (count <= 0) {
	throw new EOFException();
	}
	return count;
	}

The returned count would be buffer.position() then.

The method readInBuffer is also called by compare. But that method might be affected by the same problem.

[philwebb]

Thanks very much for the detailed analysis @jhackel-hypo!

I think I've pushed something that will fix this. I'm not keen to move DataBlock#readyFully because those methods are called a lot and I want to keep things as fast and lean as possible.