Uploaded image for project: 'Minecraft: Java Edition'
  1. Minecraft: Java Edition
  2. MC-208990

Resource leak in RegionFile

XMLWordPrintable

    • Icon: Bug Bug
    • Resolution: Unresolved
    • None
    • 1.16.4, 20w51a, 1.18 Release Candidate 3, 1.18, 1.18.1
    • None
    • Plausible
    • (Unassigned)

      This is a bit of a technical issue that is related to how minecraft is developed rather than an actual bug, and I didn't report this because a feature is broken but because resource leaks are bad and must be prevented at all times.

      The issue

      In net.minecraft.world.level.chunk.storage.RegionFile, there is a resource leak, which I found after decompiling 20w51a. Have a look at the following two methods in RegionFile:

       

      @Nullable
      private DataInputStream createChunkInputStream(ChunkPos pos, 
                                                     byte versionId,
                                                     InputStream in) 
                                                     throws IOException {
         RegionFileVersion version = RegionFileVersion.fromId(versionId);
         if(version == null) {
            LOGGER.error(
               "Chunk {} has invalid chunk stream version {}", 
               pos, Byte.valueOf(versionId));
            // Oh no: 'in' never get's closed. Resource leak found!
            // But in this case we would not even need to open the resource
            // This malformed version can also be catched BEFORE
            // calling 'createChunkInputStream', so that no resource has to be opened
            // at all
            return null;
         } else {
            return new DataInputStream(new BufferedInputStream(version.wrap(in)));
         }
      }
      
      @Nullable
      private DataInputStream createExternalChunkInputStream(ChunkPos pos, byte versionId) throws IOException {
         Path extPath = getExternalChunkPath(pos);
         if(!Files.isRegularFile(extPath)) {
            LOGGER.error("External chunk path {} is not file", extPath);
            return null;
         } else {
            return createChunkInputStream(
               pos, versionId, 
               // v  Newly opened input stream, which createChunkInputStream
               // v  doesn't close upon error!
               Files.newInputStream(extPath));
         }
      }
      

       
      If no region files have been corrupted, this resource leak does not happen, but at the point of corrupted region files multiple InputStreams will be opened and not closed.

      A possible fix

      To help fixing the bug, here is a probable fix:

      In getChunkDataInputStream this check for the correct compression type can also be performed. I propose a few changes in that method:

      @Nullable
      public synchronized DataInputStream 
      getChunkDataInputStream(ChunkPos pos) throws IOException {
         int off = getOffset(pos);
         if(off == 0) {
            return null;
         } else {
            int num = getSectorNumber(off);
            int secs = getNumSectors(off);
            int secBytes = secs * 4096;
      
            ByteBuffer buf = ByteBuffer.allocate(secBytes);
            file.read(buf, num * 4096L);
            buf.flip();
      
            if(buf.remaining() < 5) {
               LOGGER.error(
                   "Chunk {} header is truncated: expected {} but read {}",
                   pos,
                   Integer.valueOf(secBytes),
                   Integer.valueOf(buf.remaining()));
               return null;
            } else {
               int unpaddedBytes = buf.getInt();
               byte header = buf.get();
      
               // Instead of checking this in createChunkInputStream, 
               // check it before opening any resource here
               byte version = getExternalChunkVersion(header);
               if(!RegionFileVersion.isValidVersion(version)) {
                  LOGGER.error("Chunk {} has invalid chunk stream version {}", 
                               pos, version);
                  return null;
               }
      
               if(unpaddedBytes == 0) {
                  LOGGER.warn("Chunk {} is allocated, but stream is missing", pos);
                  return null;
               } else {
                  int payloadBytes = unpaddedBytes - 1;
                  if(isExternalStreamChunk(header)) {
                     if(payloadBytes != 0) {
                        LOGGER.warn(
                            "Chunk has both internal and external streams");
                     }
      
                     // Use 'version' here for ease, we already computed this
                     return createExternalChunkInputStream(pos, version);
                  } else if(payloadBytes > buf.remaining()) {
                     LOGGER.error(
                         "Chunk {} stream is truncated: expected {} but read {}", 
                         pos, Integer.valueOf(payloadBytes), 
                         Integer.valueOf(buf.remaining()));
                     return null;
                  } else if(payloadBytes < 0) {
                     LOGGER.error(
                         "Declared size {} of chunk {} is negative", 
                         Integer.valueOf(unpaddedBytes), pos);
                     return null;
                  } else {
                     // Use 'version' here for ease, we already computed this
                     return createChunkInputStream(
                         pos, version,
                         createStream(buf, payloadBytes));
                  }
               }
            }
         }
      }
      

      (all sources here are mapped in official mappings and decompiled with Fernflower, variables have been named by me for clarity in the code samples)

            Unassigned Unassigned
            RGSW Samū
            Votes:
            4 Vote for this issue
            Watchers:
            3 Start watching this issue

              Created:
              Updated:
              CHK: